Add running workflow for worker index to cache #599

micahhausler · 2022-03-22T18:45:30Z

Description

Adds indexer so clients can query for running workflows by a particular worker address

Signed-off-by: Micah Hausler [email protected]

Why is this needed

Boots needs to import this logic and query for these workflows

Fixes: # N/A

How Has This Been Tested?

Added unit tests for existing and new index functions

How are existing users impacted? What migration steps/scripts do we need?

N/A

Checklist:

I have:

updated the documentation and/or roadmap (if required)
added unit or e2e tests
provided instructions on how to upgrade

codecov · 2022-03-22T18:51:04Z

Codecov Report

Merging #599 (eb40dae) into main (1d4fd95) will decrease coverage by 0.26%.
The diff coverage is 61.40%.

@@            Coverage Diff             @@
##             main     #599      +/-   ##
==========================================
- Coverage   45.94%   45.68%   -0.27%     
==========================================
  Files          53       55       +2     
  Lines        3182     3301     +119     
==========================================
+ Hits         1462     1508      +46     
- Misses       1634     1707      +73     
  Partials       86       86

Impacted Files	Coverage Δ
pkg/controllers/manager.go	`40.00% <61.40%> (ø)`
pkg/controllers/retry.go	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d4fd95...eb40dae. Read the comment docs.

displague

This looks good and is consistent with our existing index.

I have some questions about the format of keys (should they start with a .), names of meta keys (is the snake case field name conventional), and return values (Indexer documentation says only return a single string for Kubernetes compatibility), but our existing index brings up the same questions. (I wont hold this up on that)

https://github.com/kubernetes-sigs/controller-runtime/blob/v0.11.1/pkg/client/interfaces.go#L125-L136

micahhausler · 2022-03-22T21:07:57Z

I have some questions about the format of keys (should they start with a .),

I'm happy to prefix with a period

names of meta keys (is the snake case field name conventional)

Good catch, I'll fix this to be more inline with the API field name convention

and return values (Indexer documentation says only return a single string for Kubernetes compatibility), but our existing index brings up the same questions.

I believe this only matters if you wanted to compatibility with a field-selector in the API, which we don't here. We intentionally want all the workers for a particular workflow.

detiber

A couple of questions on the index itself, but I'm curious why this is part of pkg/controllers/manager.go if it's expected to be consumed by boots or other clients, would it make more sense to have it in a separate package to be consumed by the clients?

detiber · 2022-03-23T19:38:15Z

pkg/controllers/manager.go

@@ -17,7 +17,8 @@ import (
 )

 const (
-	WorkerAddr = "status.tasks.workeraddr"
+	WorkerAddrIndex       = ".status.tasks.workerAddr"
+	NonTerminalStateIndex = ".status.state.nonTerminalWorker"


Curious to see if it would make sense to break this out on a per-state basis instead of just non-terminal states.

I'm also wondering if we might run into issues based on the field naming here, given the comment about the FieldIndexer interface here: https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/client#FieldIndexer

Curious to see if it would make sense to break this out on a per-state basis instead of just non-terminal states

I've added an additional index for state, WorkflowStateIndex and I've updated the index variable names for clarity. Take a look at the documented example under FieldIndexer, the Index name is just a key to lookup an object based on shared understanding of what that index means. In the documented example, the field name is spec.volumes.secret.secretName and returns all the secret names mounted on a pod.

micahhausler · 2022-03-24T19:41:21Z

A couple of questions on the index itself, but I'm curious why this is part of pkg/controllers/manager.go if it's expected to be consumed by boots or other clients, would it make more sense to have it in a separate package to be consumed by the clients?

Is there anywhere else you're thinking of specifically? This code is importable by Boots in the current package. Boots will become just a client of Kubernetes, as opposed to the Tinkerbell API. In that architecture, Boots will need still need indexes on Hardware by MAC and IP, as well as workflows for a particular worker.

thebsdbox

This looks OK to me, based upon reading through the code and a review of the comments. Unless @detiber has any strong feelings on the convo re:boots.

mmlb

lgtm after the func comment is fixed

pkg/controllers/manager.go

mmlb · 2022-04-04T21:59:23Z

After taking a look at #600, does it make more sense to get that merged first and then use the defined constants instead of bare strings here?

micahhausler · 2022-04-04T22:03:13Z

After taking a look at #600, does it make more sense to get that merged first and then use the defined constants instead of bare strings here?

Yep! I'll rebase this and reference the constants once #600 merges

* Add tests on index functions * Add constants for action states Signed-off-by: Micah Hausler <[email protected]>

mmlb

lgtm!

micahhausler force-pushed the controller/indexer branch from b42a5c2 to 58a3754 Compare March 22, 2022 18:49

micahhausler force-pushed the controller/indexer branch from 58a3754 to 26dcb89 Compare March 22, 2022 18:54

micahhausler requested review from displague, mmlb and detiber March 22, 2022 19:06

displague previously approved these changes Mar 22, 2022

View reviewed changes

micahhausler dismissed displague’s stale review via 1e2fb3f March 22, 2022 21:10

micahhausler force-pushed the controller/indexer branch from 26dcb89 to 1e2fb3f Compare March 22, 2022 21:10

displague previously approved these changes Mar 22, 2022

View reviewed changes

detiber reviewed Mar 23, 2022

View reviewed changes

micahhausler dismissed displague’s stale review via 1d9550b March 24, 2022 19:44

micahhausler force-pushed the controller/indexer branch 3 times, most recently from a40de6e to 13455f4 Compare March 24, 2022 21:51

thebsdbox previously approved these changes Mar 30, 2022

View reviewed changes

mmlb reviewed Apr 4, 2022

View reviewed changes

pkg/controllers/manager.go Outdated Show resolved Hide resolved

micahhausler dismissed thebsdbox’s stale review via 90101b6 April 4, 2022 21:58

micahhausler force-pushed the controller/indexer branch from 13455f4 to 90101b6 Compare April 4, 2022 21:58

Add running workflow for worker index to cache

eb40dae

* Add tests on index functions * Add constants for action states Signed-off-by: Micah Hausler <[email protected]>

micahhausler force-pushed the controller/indexer branch from 90101b6 to eb40dae Compare April 4, 2022 22:23

mmlb approved these changes Apr 4, 2022

View reviewed changes

mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Apr 4, 2022

mergify bot merged commit dbe434f into tinkerbell:main Apr 4, 2022

micahhausler deleted the controller/indexer branch April 4, 2022 22:40

displague added this to the 0.7.0 milestone Aug 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add running workflow for worker index to cache #599

Add running workflow for worker index to cache #599

micahhausler commented Mar 22, 2022

codecov bot commented Mar 22, 2022 •

edited

Loading

displague left a comment •

edited

Loading

micahhausler commented Mar 22, 2022

detiber left a comment

detiber Mar 23, 2022

micahhausler Mar 24, 2022

micahhausler commented Mar 24, 2022

thebsdbox left a comment

mmlb left a comment

mmlb commented Apr 4, 2022

micahhausler commented Apr 4, 2022

mmlb left a comment

Add running workflow for worker index to cache #599

Add running workflow for worker index to cache #599

Conversation

micahhausler commented Mar 22, 2022

Description

Why is this needed

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

codecov bot commented Mar 22, 2022 • edited Loading

Codecov Report

displague left a comment • edited Loading

Choose a reason for hiding this comment

micahhausler commented Mar 22, 2022

detiber left a comment

Choose a reason for hiding this comment

detiber Mar 23, 2022

Choose a reason for hiding this comment

micahhausler Mar 24, 2022

Choose a reason for hiding this comment

micahhausler commented Mar 24, 2022

thebsdbox left a comment

Choose a reason for hiding this comment

mmlb left a comment

Choose a reason for hiding this comment

mmlb commented Apr 4, 2022

micahhausler commented Apr 4, 2022

mmlb left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 22, 2022 •

edited

Loading

displague left a comment •

edited

Loading