Add pod level inject webhook #716

wuchunghsuan · 2019-08-12T10:17:53Z

For issue #685

This change is

wuchunghsuan · 2019-08-12T10:19:11Z

gaocegege · 2019-08-13T01:32:45Z

pkg/webhook/v1alpha2/experiment/inject_webhook.go

+	"github.com/kubeflow/katib/pkg/webhook/v1alpha2/experiment/injector"
+)
+
+// experimentDefaulter that sets default fields in experiment


nit: Please update the comment

hougangliu · 2019-08-13T09:50:07Z

pkg/webhook/v1alpha2/experiment/inject_webhook.go

@@ -0,0 +1,72 @@
+/*


can you add this webhook under pkg/webhook/v1alpha2/pod

hougangliu · 2019-08-15T02:33:05Z

pkg/webhook/v1alpha2/webhook.go

@@ -84,5 +86,16 @@ func register(manager manager.Manager, server *webhook.Server) error {
 	if err != nil {
 		return err
 	}
-	return server.Register(mutatingWebhook, validatingWebhook)
+	injectWebhook, err := builder.NewWebhookBuilder().
+		Name("mutating.pod.kubeflow.org").


I think maybe we can rename it to one which is katib related

Yes, do you have any suggestion about the name?

I think we should follow xx.$group.$domain style (group should be katib and domain is kubeflow.org here), we'd better make all others consistent with this style.

SGTM, it is a historical problem. Can we do the change in another PR? I think we could keep the name here and update it after the PR is merged, with other changes.

gaocegege · 2019-08-26T03:31:38Z

@hougangliu Please take a look. We succeeded to use the modified metrics collector to get the metrics.

hougangliu · 2019-08-26T04:01:14Z

@hougangliu Please take a look. We succeeded to use the modified metrics collector to get the metrics.

cool! I will look at it later today soon

hougangliu · 2019-08-26T05:51:08Z

pkg/util/v1alpha2/metricscollector/metricscollector.go

-	logs, err := d.clientset.CoreV1().Pods(namespace).GetLogs(pl.Items[0].ObjectMeta.Name, &logopt).Do().Raw()
-	if err != nil {
-		return nil, err
+	logopt := apiv1.PodLogOptions{Container: "tensorflow", Timestamps: true, Follow: true}


Only works for "tfjob"?

Only a PoC now.

hougangliu · 2019-08-26T05:58:28Z

pkg/common/v1alpha2/common.go

@@ -41,8 +41,8 @@ func GetJobLabelMap(jobKind string, trialName string) map[string]string {
 	labelMap := make(map[string]string)

 	if jobKind == "TFJob" {
-		labelMap["tf-job-name"] = trialName
-		labelMap["tf-job-role"] = "master"
+		labelMap["job-name"] = trialName


kubeflow/pytorch-operator#204 had made pytorch also can use "job-name"

Done, updated the pytorch label name.

hougangliu · 2019-08-26T06:05:54Z

pkg/webhook/v1alpha2/pod/inject_webhook.go

+}
+
+func (s *sidecarInjector) MutationRequired(pod *v1.Pod) bool {
+	value, err := s.GetLabel(pod, JobRoleLabel)


it can handle tfjob and pytorchjob case, but not cover k8s Job case

Yeah, it is just a PoC. In the future, we will support job

hougangliu · 2019-08-26T06:08:24Z

pkg/webhook/v1alpha2/pod/inject_webhook.go

+	// Get the namespace from req since the namespace in the pod is empty.
+	namespace := req.AdmissionRequest.Namespace
+	// Do mutation
+	mutatedPod, err := s.Mutate(pod, namespace)


namespace can be get from pod object, so I think we can just use one argument instead, right?

According to our experiment, we fail to get the namespace from pod.

hougangliu · 2019-08-26T06:13:23Z

pkg/webhook/v1alpha2/pod/inject_webhook.go

+		VolumeMounts:    pod.Spec.Containers[0].VolumeMounts,
+	}
+	mutatedPod.Spec.Containers = append(mutatedPod.Spec.Containers, injectContainer)
+	mutatedPod.Spec.ServiceAccountName = pod.Spec.ServiceAccountName


why we need line 149, line 120 cannot assign ServiceAccountName?

It seems that we cannot get it. There is a hack.

cc @wuchunghsuan

hougangliu · 2019-08-26T06:15:24Z

pkg/webhook/v1alpha2/pod/inject_webhook.go

+		Name:            "metrics-collector",
+		Image:           "gcr.io/kubeflow-images-public/katib/v1alpha2/metrics-collector",
+		Command:         []string{"./metricscollector"},
+		Args:            []string{"-e", "TODO_Experiment", "-t", trialName, "-k", "TFJob", "-n", namespace, "-m", katibmanagerv1alpha2.GetManagerAddr(), "-mn", metricName},


we can use "controller-name" label of Pod to get the "-k" value
we can get "-e" value, that is the experiment name, from trial's metadata ownerReferences field

Done, got "-k" and "-e" values. It seems pytorch job does not have "controller-name" label yet?

googlebot · 2019-08-28T03:52:52Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2019-08-28T03:59:19Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

gaocegege · 2019-08-28T05:48:03Z

pkg/util/v1alpha2/metricscollector/metricscollector.go

@@ -46,18 +45,15 @@ func (d *MetricsCollector) CollectObservationLog(tId string, jobKind string, met
 	if len(pl.Items) == 0 {
 		return nil, fmt.Errorf("No Pods are found in Trial %v", tId)
 	}
-	logopt := apiv1.PodLogOptions{Container: "tensorflow", Timestamps: true, Follow: true}


I think we should keep the file unmodified since we have pkg/util/v1alpha2/sidecarmetricscollector

gaocegege · 2019-08-28T05:49:18Z

Please remove [WIP] in the title

wuchunghsuan · 2019-08-28T06:09:34Z

OK, ready to merge.

hougangliu · 2019-08-28T06:29:12Z

/lgtm
we cab merge it now to enhance based on it

hougangliu · 2019-08-28T06:48:35Z

/approve

k8s-ci-robot · 2019-08-28T06:48:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hougangliu

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~~ [hougangliu]

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

Add pod level inject webhook.

aabeba0

k8s-ci-robot added the do-not-merge/work-in-progress label Aug 12, 2019

k8s-ci-robot requested review from Akado2009 and andreyvelich August 12, 2019 10:18

k8s-ci-robot added the size/M label Aug 12, 2019

k8s-ci-robot requested a review from gaocegege August 12, 2019 10:19

gaocegege reviewed Aug 13, 2019

View reviewed changes

hougangliu reviewed Aug 13, 2019

View reviewed changes

Implement sidecar injection (Hard code container)

8cccde6

k8s-ci-robot added size/L and removed size/M labels Aug 14, 2019

hougangliu reviewed Aug 15, 2019

View reviewed changes

gaocegege mentioned this pull request Aug 15, 2019

[enhancement] Update the group name in katib #719

Closed

wuchunghsuan added 3 commits August 19, 2019 10:25

Inject metrics collector as a sidecar

bb42b18

Update metrics-collector to satisfy sidecar

4f06a88

Clean up test logs

5f9e8b5

hougangliu reviewed Aug 26, 2019

View reviewed changes

wuchunghsuan added 3 commits August 27, 2019 12:23

Get experiment name and job kind

e2831e7

Update common labels

9dbb823

Separate the sidecar metrics collector

12608c5

k8s-ci-robot added size/XL and removed size/L labels Aug 28, 2019

wuchunghsuan force-pushed the pod-webhook branch from 02c885e to 0a08f6d Compare August 28, 2019 03:57

k8s-ci-robot added size/XXL and removed size/XL labels Aug 28, 2019

wuchunghsuan force-pushed the pod-webhook branch from 0a08f6d to 12608c5 Compare August 28, 2019 03:59

k8s-ci-robot added size/L and removed size/XXL labels Aug 28, 2019

Merge remote-tracking branch 'upstream/master' into pod-webhook

c7c5aab

gaocegege reviewed Aug 28, 2019

View reviewed changes

wuchunghsuan changed the title ~~[WIP] Add pod level inject webhook~~ Add pod level inject webhook Aug 28, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress label Aug 28, 2019

k8s-ci-robot assigned hougangliu Aug 28, 2019

k8s-ci-robot added the lgtm label Aug 28, 2019

k8s-ci-robot added the approved label Aug 28, 2019

k8s-ci-robot merged commit 1945560 into kubeflow:master Aug 28, 2019

hougangliu mentioned this pull request Dec 12, 2019

Remove redundant serviceAccountName assignment #969

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pod level inject webhook #716

Add pod level inject webhook #716

wuchunghsuan commented Aug 12, 2019 •

edited by jlewi

Loading

wuchunghsuan commented Aug 12, 2019

gaocegege Aug 13, 2019

hougangliu Aug 13, 2019

hougangliu Aug 15, 2019

gaocegege Aug 15, 2019

hougangliu Aug 15, 2019

gaocegege Aug 15, 2019

hougangliu Aug 15, 2019

gaocegege commented Aug 26, 2019

hougangliu commented Aug 26, 2019

hougangliu Aug 26, 2019

gaocegege Aug 26, 2019

hougangliu Aug 26, 2019

wuchunghsuan Aug 27, 2019

hougangliu Aug 26, 2019

gaocegege Aug 26, 2019

hougangliu Aug 26, 2019

gaocegege Aug 26, 2019

hougangliu Aug 26, 2019

gaocegege Aug 26, 2019

hougangliu Aug 26, 2019

wuchunghsuan Aug 27, 2019

googlebot commented Aug 28, 2019

googlebot commented Aug 28, 2019

gaocegege Aug 28, 2019

gaocegege commented Aug 28, 2019

wuchunghsuan commented Aug 28, 2019

hougangliu commented Aug 28, 2019

hougangliu commented Aug 28, 2019

k8s-ci-robot commented Aug 28, 2019

Add pod level inject webhook #716

Add pod level inject webhook #716

Conversation

wuchunghsuan commented Aug 12, 2019 • edited by jlewi Loading

wuchunghsuan commented Aug 12, 2019

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

gaocegege commented Aug 26, 2019

hougangliu commented Aug 26, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Aug 28, 2019

googlebot commented Aug 28, 2019

Choose a reason for hiding this comment

gaocegege commented Aug 28, 2019

wuchunghsuan commented Aug 28, 2019

hougangliu commented Aug 28, 2019

hougangliu commented Aug 28, 2019

k8s-ci-robot commented Aug 28, 2019

wuchunghsuan commented Aug 12, 2019 •

edited by jlewi

Loading