-
Notifications
You must be signed in to change notification settings - Fork 138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reworked phase/status-field handling #609
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was logged in with the team user and only noticed after writing the comments
- Michael
return | ||
} | ||
dkState.Update(upd, defaultUpdateInterval, "infra monitoring reconciled") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dkState.Update
is a bit weird, at this point its a logging util
also the last setting of the interval will win, (also if we always use defaultUpdateInterval
always then why pass it in 😅 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On that note, afaik, upd
is a boolean. Assuming defaultUpdateInterval
is refactored to a constant, it would seem prudent to combine the two remaining parameters into a struct, which may or may not also take dkState
, and call Update with it or, if dkState
is also in that struct, call it as a receiver function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the interval parameter should be removed as it does not make sense.
I think it's fine that it's kind of a logging util, as it adds the benefit that it sets the updated field for you on updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For combining the 2 variables into a struct, what would be our advantage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For combining the 2 variables into a struct, what would be our advantage?
increased read- and maintainability of the code base.
Also it's prudent to combine parameters into logical units, i.e., structs, and reduce parameter counts of functions.
Description
In our current implementation the phase dynakube status field should indicate if all oneagents were deployed successfully. This was not the case because our code was searching for an old daemonset name which was no longer there. Therefore if the field was once set to error, it was never reseted. The bug was fixed in this PR.
Additionally, the operator and the deployment of the dynakube is more than just generating oneagents => Advanced the phase field checking: Now, the dynakube status field is
To quickly show the state of the deployment, the phase field was added to
additionalPrinterColumns
in our dynakube crd.Besides of that, a reconciliation of the dynakube was reworked a little but, to decrease the number of reconciliation the operator needs and unit tests for the dynakube reconciler were added.
How can this be tested?
Apply the new CRD and start in a monitoring mode you prefer.
In the operator logs you can see:
With
kubectl get dynakube
you should see the additional printer column.Checklist