This repository was archived by the owner on Mar 16, 2021. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added Kustomize for controller, sidecar & CRDs #9
Added Kustomize for controller, sidecar & CRDs #9
Changes from all commits
53cfe8c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The
cosi-controller-manager
does not in any way require thesample-provisioner
to be running in the cluster, as such, its deployment should not include it. At least, thebase
layer. This is a case where having an overlay which does deploy the whole stack including thesample-provisioner
could make sense, for testing/demo purposes!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 one hand, the image overrides specify the
latest
image tags, but then we'd useIfNotPresent
asImagePullPolicy
, that seems weird (in general, you wantlatest
to beAlways
).Now, K8s will automatically use
Always
forlatest
images, andIfNotPresent
for others, so there's no reason (I believe) to have these patches. Let's go with the default, and if there are site-specific overrides required, one can do this in an overlay on this base.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.
This could be coded in the manifest directly, no reason to do it as a patch.
Also, consider adopting https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/ instead.
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.
Let's not handle namespaces like this but rely on
kubectl apply -n ...
and alike. See earlier comment.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.
Same comment as above.
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.
Same comment as above.
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.
Objects can get prefixed through a
kustomization
if desired, so no real need to do so here. Plaincontroller
should do.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.
See earlier comments.
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.
Does the controller have some kind of leader election? If not, I think it's easier to simply deploy this as a
StatefulSet
of a single replica.Keep in mind a
Deployment
withreplicas: 1
does not guarantee only a single replica is running at any point in time.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.
Unless leader election is implemented, this could cause trouble 😃
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.
We have leader election
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.
Let's be a bit more specific, e.g.:
Then, have a
app.kubernetes.io/instance
in the rootKustomization
, e.g. set tocosi-controller-manager
(as well).Again, see https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/#labels.
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.
See above. Here, have a
app.kubernetes.io/version
as well.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.
See earlier comment about prefix handling.
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.
No need, see above.