feature: Add group size environment variable injection #208

googs1025 · 2024-09-04T04:54:42Z

What type of PR is this?

/kind feature

What this PR does / why we need it

Add group size environment variable injection

Which issue(s) this PR fixes

Fixes # #203

Special notes for your reviewer

Does this PR introduce a user-facing change?

Add LWS_GROUP_SIZE environment variable to all containers in a leaderworkerset.

googs1025 · 2024-09-04T05:19:42Z

Practical use of local branches

root@VM-0-8-ubuntu:/home/ubuntu/lws# kubectl get pods
NAME                         READY   STATUS    RESTARTS   AGE
leaderworkerset-sample-0     1/1     Running   0          32s
leaderworkerset-sample-0-1   1/1     Running   0          32s
leaderworkerset-sample-0-2   1/1     Running   0          32s
leaderworkerset-sample-0-3   1/1     Running   0          32s
leaderworkerset-sample-1     1/1     Running   0          32s
leaderworkerset-sample-1-1   1/1     Running   0          32s
leaderworkerset-sample-1-2   1/1     Running   0          32s
leaderworkerset-sample-1-3   1/1     Running   0          32s
leaderworkerset-sample-2     1/1     Running   0          32s
leaderworkerset-sample-2-1   1/1     Running   0          32s
leaderworkerset-sample-2-2   1/1     Running   0          32s
leaderworkerset-sample-2-3   1/1     Running   0          32s
root@VM-0-8-ubuntu:/home/ubuntu/lws# kubectl describe pods leaderworkerset-sample-2
Name:             leaderworkerset-sample-2
Namespace:        default
Priority:         0
Service Account:  default
Node:             cluster1-worker2/172.18.0.3
Start Time:       Wed, 04 Sep 2024 13:17:19 +0800
Labels:           apps.kubernetes.io/pod-index=2
                  controller-revision-hash=leaderworkerset-sample-7b67975f84
                  leaderworkerset.sigs.k8s.io/group-index=2
                  leaderworkerset.sigs.k8s.io/group-key=ee0d9ef1ac6e34b19b82b1ec3c9df8128b804444
                  leaderworkerset.sigs.k8s.io/name=leaderworkerset-sample
                  leaderworkerset.sigs.k8s.io/template-revision-hash=97c9d753bb0b5993c9154ee9b9ea554e2ed3fb3d
                  leaderworkerset.sigs.k8s.io/worker-index=0
                  statefulset.kubernetes.io/pod-name=leaderworkerset-sample-2
Annotations:      leaderworkerset.sigs.k8s.io/size: 4
Status:           Running
IP:               10.6.2.13
IPs:
  IP:           10.6.2.13
Controlled By:  StatefulSet/leaderworkerset-sample
Containers:
  nginx:
    Container ID:   containerd://c40aa91f558e0d045551bc14e4a805490eba881244c7e50b51d73f9e576bf5e7
    Image:          nginx:1.14.2
    Image ID:       docker.io/library/nginx@sha256:f7988fb6c02e0ce69257d9bd9cf37ae20a60f1df7563c3a2a6abe24160306b8d
    Port:           8080/TCP
    Host Port:      0/TCP
    State:          Running
      Started:      Wed, 04 Sep 2024 13:17:20 +0800
    Ready:          True
    Restart Count:  0
    Limits:
      cpu:  100m
    Requests:
      cpu:  50m
    Environment:
      LWS_LEADER_ADDRESS:  leaderworkerset-sample-2.leaderworkerset-sample.default
      LWS_GROUP_SIZE:      4
    Mounts:
      /var/run/secrets/kubernetes.io/serviceaccount from kube-api-access-fc46f (ro)
Conditions:
  Type                        Status
  PodReadyToStartContainers   True
  Initialized                 True
  Ready                       True
  ContainersReady             True
  PodScheduled                True
Volumes:
  kube-api-access-fc46f:
    Type:                    Projected (a volume that contains injected data from multiple sources)
    TokenExpirationSeconds:  3607
    ConfigMapName:           kube-root-ca.crt
    ConfigMapOptional:       <nil>
    DownwardAPI:             true
QoS Class:                   Burstable
Node-Selectors:              <none>
Tolerations:                 node.kubernetes.io/not-ready:NoExecute op=Exists for 300s
                             node.kubernetes.io/unreachable:NoExecute op=Exists for 300s
Events:
  Type    Reason     Age   From               Message
  ----    ------     ----  ----               -------
  Normal  Scheduled  40s   default-scheduler  Successfully assigned default/leaderworkerset-sample-2 to cluster1-worker2
  Normal  Pulled     39s   kubelet            Container image "nginx:1.14.2" already present on machine
  Normal  Created    39s   kubelet            Created container nginx
  Normal  Started    39s   kubelet            Started container nginx
root@VM-0-8-ubuntu:/home/ubuntu/lws#

kannon92 · 2024-09-04T13:16:49Z

pkg/utils/pod/pod_utils.go

@@ -119,11 +125,24 @@ func AddLWSVariables(pod *corev1.Pod) error {
 		Value: fmt.Sprintf("%s-%s.%s.%s", lwsName, groupIndex, lwsName, pod.ObjectMeta.Namespace),
 	}

+	size, fount := pod.Annotations[leaderworkerset.SizeAnnotationKey]


Suggested change

size, fount := pod.Annotations[leaderworkerset.SizeAnnotationKey]

size, found := pod.Annotations[leaderworkerset.SizeAnnotationKey]

kannon92 · 2024-09-04T13:17:23Z

pkg/utils/pod/pod_utils.go

@@ -119,11 +125,24 @@ func AddLWSVariables(pod *corev1.Pod) error {
 		Value: fmt.Sprintf("%s-%s.%s.%s", lwsName, groupIndex, lwsName, pod.ObjectMeta.Namespace),
 	}

+	size, fount := pod.Annotations[leaderworkerset.SizeAnnotationKey]
+	if !fount {
+		return fmt.Errorf("Failure constructing environment variables, no size annotation found for pod %v", pod.Name)


Suggested change

return fmt.Errorf("Failure constructing environment variables, no size annotation found for pod %v", pod.Name)

return fmt.Errorf("Failure constructing environment variables, no size annotation found for pod %v", pod)

Technically the pods can have the same name if they are in a different namespace.

thanks. i use klog.KObj(pod).

kannon92 · 2024-09-04T13:18:28Z

pkg/utils/pod/pod_utils_test.go

@@ -104,36 +104,43 @@ func TestAddLWSVariables(t *testing.T) {
 		name                     string
 		pod                      *corev1.Pod
 		expectedLwsLeaderAddress string
+		expectedGroupSize        string


I think having this as an int makes more sense to me.

kannon92 · 2024-09-04T13:19:40Z

pkg/utils/pod/pod_utils_test.go

 	}{
 		{
 			name:                     "Leader pod",
-			pod:                      testutils.MakePodWithLabels("test-sample", "0", "", "default"),
+			pod:                      testutils.MakePodWithLabels("test-sample", "0", "", "default", 2),


I'd have one case where you having different than two..

Hey, Kevin. I don't quite understand what this suggestion means. Could you explain it more clearly?

use a different value from "2".

kannon92 · 2024-09-04T13:20:21Z

/retest

pkg/utils/pod/pod_utils.go

liurupeng · 2024-09-05T04:26:56Z

/lgtm
/approve

k8s-ci-robot · 2024-09-05T04:27:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: googs1025, 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:

~~OWNERS~~ [liurupeng]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 4, 2024

k8s-ci-robot requested a review from ahg-g September 4, 2024 04:54

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 4, 2024

k8s-ci-robot requested a review from liurupeng September 4, 2024 04:54

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 4, 2024

kannon92 reviewed Sep 4, 2024

View reviewed changes

googs1025 force-pushed the feature/group_var branch from 7cc5660 to 4c09205 Compare September 4, 2024 14:55

liurupeng reviewed Sep 4, 2024

View reviewed changes

pkg/utils/pod/pod_utils.go Outdated Show resolved Hide resolved

liurupeng reviewed Sep 4, 2024

View reviewed changes

pkg/utils/pod/pod_utils.go Outdated Show resolved Hide resolved

googs1025 force-pushed the feature/group_var branch from 4c09205 to 6df8fcc Compare September 5, 2024 01:00

feature: Add group size environment variable injection

44d877b

googs1025 force-pushed the feature/group_var branch from 6df8fcc to 44d877b Compare September 5, 2024 01:02

k8s-ci-robot assigned liurupeng Sep 5, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2024

k8s-ci-robot merged commit 58263b0 into kubernetes-sigs:main Sep 5, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Add group size environment variable injection #208

feature: Add group size environment variable injection #208

googs1025 commented Sep 4, 2024

googs1025 commented Sep 4, 2024

kannon92 Sep 4, 2024

googs1025 Sep 4, 2024

kannon92 Sep 4, 2024

googs1025 Sep 4, 2024

kannon92 Sep 4, 2024

googs1025 Sep 4, 2024

kannon92 Sep 4, 2024

googs1025 Sep 4, 2024

kannon92 Sep 4, 2024

googs1025 Sep 5, 2024

kannon92 commented Sep 4, 2024

liurupeng commented Sep 5, 2024

k8s-ci-robot commented Sep 5, 2024

	size, fount := pod.Annotations[leaderworkerset.SizeAnnotationKey]
	size, found := pod.Annotations[leaderworkerset.SizeAnnotationKey]

	return fmt.Errorf("Failure constructing environment variables, no size annotation found for pod %v", pod.Name)
	return fmt.Errorf("Failure constructing environment variables, no size annotation found for pod %v", pod)

feature: Add group size environment variable injection #208

feature: Add group size environment variable injection #208

Conversation

googs1025 commented Sep 4, 2024

What type of PR is this?

What this PR does / why we need it

Which issue(s) this PR fixes

Special notes for your reviewer

Does this PR introduce a user-facing change?

googs1025 commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kannon92 commented Sep 4, 2024

liurupeng commented Sep 5, 2024

k8s-ci-robot commented Sep 5, 2024