add publish not ready headless service to jobset #505
Conversation
k8s-ci-robot
added
cncf-cla: yes
✅ Deploy Preview for kubernetes-sigs-jobset canceled.
|
kannon92
force-pushed
the
publish-not-ready-headless-service
branch
from
05b6212 to
8ab1de0
Compare
|
Man this is a rough one.. I am not able to generate CRDs anymore so the integration tests are failing due to the fact that crd are not generated. Looking into this now. /hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
Makefile
Outdated
| @@ -83,6 +83,11 @@ manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and Cust | |||
| $(CONTROLLER_GEN) \ | |||
| rbac:roleName=manager-role output:rbac:artifacts:config=config/components/rbac\ | |||
| paths="./api/..." | |||
|
|
|||
| $(CONTROLLER_GEN) \ | |||
There was a problem hiding this comment.
I noticed that I wasn’t generating crds anymore. Added this allows me to generate the CRD.
not sure when this broke.
kannon92
force-pushed
the
publish-not-ready-headless-service
branch
5 times, most recently
from
18e28a2 to
cd1c961
Compare
kannon92
force-pushed
the
publish-not-ready-headless-service
branch
from
cd1c961 to
cfcf10f
Compare
|
/assign @ahg-g @danielvegamyhre |
pkg/webhooks/jobset_webhook_test.go
Outdated
| @@ -35,6 +35,8 @@ var TestPodTemplate = corev1.PodTemplateSpec{ | |||
| }, | |||
| } | |||
|
|
|||
| var defaultNetwork = &jobset.Network{EnableDNSHostnames: ptr.To(true), PublishNotReadyAddresses: ptr.To(true)} | |||
There was a problem hiding this comment.
Is this only used in TestJobSetDefaulting? If so let's move it into TestJobSetDefaulting next to the defaultSuccessPolicy and defaultStartupPolicy variables.
api/jobset/v1alpha2/jobset_types.go
Outdated
|
|
||
| // PublishNotReadyAddresses indicates that any agent | ||
| // which deals with endpoints for this Service should disregard any indications of ready/not-ready. | ||
| // Defaults to true if not set |
There was a problem hiding this comment.
"Indicates if DNS records of pods should be published before the pods are ready. Defaults to True."
|
/lgtm leaving approve to Daniel |
k8s-ci-robot
added
the
lgtm
|
This change makes sense to me, thanks for working on it. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danielvegamyhre, kannon92 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 |
k8s-ci-robot
added
the
approved
Fixes #504
Adds PublishNotReadyHeadlessService to Jobset.
Defaulting logic sets to true.
Did one optimization in network defaulting where I moved it out of a loop.