-
Notifications
You must be signed in to change notification settings - Fork 43
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
Improve Image Builder Jobs Label and Annotation #380
Conversation
…image builder job
api/cmd/api/setup.go
Outdated
@@ -144,6 +144,7 @@ func initImageBuilder(cfg *config.Config) (webserviceBuilder imagebuilder.ImageB | |||
Tolerations: cfg.ImageBuilderConfig.Tolerations, | |||
NodeSelectors: cfg.ImageBuilderConfig.NodeSelectors, | |||
MaximumRetry: cfg.ImageBuilderConfig.MaximumRetry, | |||
JobSafeToEvict: cfg.ImageBuilderConfig.JobSafeToEvict, |
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.
Nit: Shall we just call this SafeToEvict
as opposed to JobSafeToEvict
? For 2 reasons:
- Since this property only corresponds to imagebuilding and it's found under the
ImageBuilder.Config
, I feel thatSafeToEvict
is more concise. JobSafeToEvict
may cause some confusion with prediction job for the readers (like myself, the first time I read it).
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.
Got it. I understand your confusion. I have updated this part.
api/pkg/imagebuilder/imagebuilder.go
Outdated
} | ||
|
||
annotations := map[string]string{ | ||
"cluster-autoscaler.kubernetes.io/safe-to-evict": fmt.Sprint(c.config.JobSafeToEvict), |
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.
Should we conditionally set this label only when it's supposed to be "false"
? Though we don't currently plan to use the value "true"
, I'm afraid that if we do so at some point in the future, it will make it more prone to eviction than usual (doc: https://kubernetes.io/docs/reference/labels-annotations-taints/#cluster-autoscaler-kubernetes-io-safe-to-evict).
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.
Ah, TIL. Thanks!
api/pkg/imagebuilder/imagebuilder.go
Outdated
|
||
annotations := make(map[string]string) | ||
if !c.config.SafeToEvict { | ||
annotations["cluster-autoscaler.kubernetes.io/safe-to-evict"] = fmt.Sprint(c.config.SafeToEvict) |
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.
Super nit: this can just be:
annotations["cluster-autoscaler.kubernetes.io/safe-to-evict"] = "false"
Also, perhaps we can leave an inline comment on why we add this annotation conditionally, linking to the doc?
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.
LGTM. Thanks, @ariefrahmansyah !
What this PR does / why we need it:
a. Removing the duplicate of kserve-controller-manager statefulset as in kserve 0.9.0 manifest has both of statefulset and deployment
b. Removing kserve-controller-manager's cpu limits
Does this PR introduce a user-facing change?:
Checklist