-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add leader address environment variable injection #144
Conversation
|
Welcome @jasonkramberger! |
Hi @jasonkramberger. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/ok-to-test |
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.
Glad to have this. Thanks @jasonkramberger
pkg/utils/pod/pod_utils_test.go
Outdated
}, | ||
}, | ||
ObjectMeta: v1.ObjectMeta{ | ||
Name: fmt.Sprintf("%s-%s-%s", setName, groupIndex, workerIndex), |
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 think the name is not right if this is a leader pod.
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.
So if this is a wrapper, let's make it more general like pass in the name and namespace to build a Pod spec.
@@ -139,6 +139,11 @@ func (p *PodWebhook) Default(ctx context.Context, obj runtime.Object) error { | |||
return err | |||
} | |||
} | |||
|
|||
if err := podutils.AddLWSVariables(pod); err != nil { |
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.
Only needed for workerPod I think, which also worths to test.
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.
itt it may simplify some use cases (init scripts) to have this be available to the leader containers as well. I don't know if that is the most compelling use case, but imo it seems more convenient to have this standardized across all containers in the lws.
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.
actually the opposite, we need to set this in the webook for the leader, but for the workers we can set it on the worker sts. But it is fine to set it here for all.
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.
Why not localhost then? But I'm ok to keep it, it's not a big problem at this moment.
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.
localhost is not relevant here :)
test/testutils/wrappers.go
Outdated
@@ -146,6 +146,23 @@ func MakeWorkerPodSpec() corev1.PodSpec { | |||
} | |||
} | |||
|
|||
func MakeWorkerPodSpecWithInitContainer() corev1.PodSpec { |
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.
Maybe make it a PodWrapper to avoid duplicate codes.
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.
Can you link me an example? I think this may be something I tackle in the follow up PR mentioned 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.
Yes, agree.
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.
A sample of this is LeaderWorkerSetWrapper under /test/testutils/wrappers.go or https://github.com/kubernetes/kubernetes/blob/fd2d352d291bc4fb36d51e52b33a6f6849f20f35/pkg/scheduler/testing/wrappers.go#L216
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.
Thanks, going to follow up with test code refactoring PR.
@@ -139,6 +139,11 @@ func (p *PodWebhook) Default(ctx context.Context, obj runtime.Object) error { | |||
return err | |||
} | |||
} | |||
|
|||
if err := podutils.AddLWSVariables(pod); err != nil { |
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.
actually the opposite, we need to set this in the webook for the leader, but for the workers we can set it on the worker sts. But it is fine to set it here for all.
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.
General LGTM. One last comment. You can squash as well.
Adds LWS_LEADER_ADDRESS environment variable injection constructed via related LWS pod labels. This avoids making the user specify in yaml via downward api.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jasonkramberger, liurupeng The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Adds LWS_LEADER_ADDRESS environment variable injection constructed via related LWS pod labels. This avoids making the user specify in yaml via downward api.
This adds LWS_LEADER_ADDRESS environment variable injection constructed via related LWS pod labels. This avoids making the user specify in yaml via downward api.
What type of PR is this?
/kind feature
What this PR does / why we need it
Which issue(s) this PR fixes
Fixes #143
Special notes for your reviewer
Does this PR introduce a user-facing change?