Update FQDN Hostname KEP to beta #1962

javidiaz · 2020-09-01T17:30:45Z

This update should contain all information required for the KEP to be able to graduate the feature to at least beta.

k8s-ci-robot · 2020-09-01T17:30:53Z

Hi @javidiaz. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

javidiaz · 2020-09-01T17:41:26Z

/cc @dashpole @marosset @thockin

javidiaz · 2020-09-01T17:41:49Z

/assign @dchen1107

dashpole · 2020-09-01T17:43:53Z

keps/sig-node/1797-configure-fqdn-as-hostname-for-pods/README.md

+- [X] (R) Test plan is in place, giving consideration to SIG Architecture and SIG Testing input
+- [X] (R) Graduation criteria is in place
+- [X] (R) Production readiness review completed
 - [ ] Production readiness review approved


Did you ever get a PRR reviewer for this?

No. I was confused about the the review completed and approved. So I checked it because I completed the questionnaire for readiness review.

It looks like it still isn't strictly required: https://github.com/kubernetes/community/blob/master/sig-architecture/production-readiness.md#status, but it would be good to get someone to take a look.

cc @prod-readiness-approvers

Thanks, let me ask in the prod-readiness channel.

That cc didn't appear to work.

cc @wojtek-t

i think it needs slash?
/cc @wojtek-t

Sorry for large delay - looking into it now.

[And starting 1.20 we will require prod readiness to be approved by PRR team.]

@wojtek-t Should I update this PR with an X in "Production readiness review approved"? or is this typically done by you guys after this merges?

Yes - feel free to mark it.

danwinship · 2020-09-02T13:31:22Z

/retitle Update FQDN Hostname KEP to beta

javidiaz · 2020-09-14T18:25:06Z

e2e tests have been successfully running for a while now in the testgrid https://k8s-testgrid.appspot.com/sig-node-kubelet#node-kubelet-alpha

Can I have some eyes on this review please? I also need ok-to-test. thanks!

marosset · 2020-09-16T22:18:53Z

/ok-to-test

keps/sig-node/1797-configure-fqdn-as-hostname-for-pods/kep.yaml

keps/sig-node/1797-configure-fqdn-as-hostname-for-pods/README.md

wojtek-t · 2020-09-18T10:14:42Z

keps/sig-node/1797-configure-fqdn-as-hostname-for-pods/README.md

+      This issue will be logged in Error level log messages. The error will something like`GeneratePodSandboxConfig for pod foo failed: Failed to construct FQDN from pod hostname and cluster domain, FQDN <long-fqdn> is too long (64 characters is the max, 70 characters requested)`      
+
    - Testing: Are there any tests for failure mode? If not describe why.
+      Unittests cover this failure scenario. The e2e tests also check for this error, but only as protection for the CI. A test that purposely hit this issue will be flaky as the pod will remain in Pending status logging errors until the test timesout.


I don't understand this - why the test should be flaky?

The test could be:

create a pod (that would exceed this limit)

watch for events and ensure the even for fdqn is delivered

once event is delivered, delete the pod

We have similar kind of tests and I don't think they are really flaky.

Oh ok, I will try to make one like you suggest.

Cool. Once done, please update this part here.

Please update - the "flaky" part doesn't make sense to me (as I wrote above). You can describe that test in testing section or sth like that.

Yes, I will update the section saying e2e also covers failure mode so I don't forget.
However, I am still working to get the e2e test right, I am working based on an AppArmor e2e test I found, i think it does what you suggested. kubernetes/kubernetes#95187

keps/sig-node/1797-configure-fqdn-as-hostname-for-pods/kep.yaml

keps/sig-node/1797-configure-fqdn-as-hostname-for-pods/README.md

wojtek-t · 2020-09-21T07:28:27Z

keps/sig-node/1797-configure-fqdn-as-hostname-for-pods/README.md

+      This issue will be logged in Error level log messages. The error will something like`GeneratePodSandboxConfig for pod foo failed: Failed to construct FQDN from pod hostname and cluster domain, FQDN <long-fqdn> is too long (64 characters is the max, 70 characters requested)`      
+
    - Testing: Are there any tests for failure mode? If not describe why.
+      Unittests cover this failure scenario. The e2e tests also check for this error, but only as protection for the CI. A test that purposely hit this issue will be flaky as the pod will remain in Pending status logging errors until the test timesout.


Cool. Once done, please update this part here.

wojtek-t

Just some minor final comments - other than those LGTM.

keps/sig-node/1797-configure-fqdn-as-hostname-for-pods/README.md

wojtek-t · 2020-09-30T12:50:27Z

keps/sig-node/1797-configure-fqdn-as-hostname-for-pods/README.md

+      This issue will be logged in Error level log messages. The error will something like`GeneratePodSandboxConfig for pod foo failed: Failed to construct FQDN from pod hostname and cluster domain, FQDN <long-fqdn> is too long (64 characters is the max, 70 characters requested)`      
+
    - Testing: Are there any tests for failure mode? If not describe why.
+      Unittests cover this failure scenario. The e2e tests also check for this error, but only as protection for the CI. A test that purposely hit this issue will be flaky as the pod will remain in Pending status logging errors until the test timesout.


Please update - the "flaky" part doesn't make sense to me (as I wrote above). You can describe that test in testing section or sth like that.

wojtek-t · 2020-09-30T14:10:19Z

/lgtm

@dchen1107 - can you please approve?

SergeyKanzhelev · 2020-10-01T20:27:31Z

keps/sig-node/1797-configure-fqdn-as-hostname-for-pods/kep.yaml

 reviewers:
  - "@thockin"
  - "@marosset"
+  - "@dashpole"


@dashpole is concentrated on instrumentation part of the k8s now. Maybe this name can be used for historical perspective for alpha version review.

keps/sig-node/1797-configure-fqdn-as-hostname-for-pods/README.md

SergeyKanzhelev · 2020-10-01T20:31:28Z

/lgtm

Co-authored-by: Sergey Kanzhelev <[email protected]>

wojtek-t

@javidiaz - please also squash commits

wojtek-t · 2020-10-02T11:39:01Z

keps/sig-node/1797-configure-fqdn-as-hostname-for-pods/README.md

+- [X] (R) Test plan is in place, giving consideration to SIG Architecture and SIG Testing input
+- [X] (R) Graduation criteria is in place
+- [X] (R) Production readiness review completed
 - [ ] Production readiness review approved


Yes - feel free to mark it.

dchen1107 · 2020-10-02T15:07:14Z

/lgtm
/approve

k8s-ci-robot · 2020-10-02T15:07:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, javidiaz

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:

~~keps/sig-node/OWNERS~~ [dchen1107]

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 1, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 1, 2020

k8s-ci-robot requested review from dchen1107 and derekwaynecarr September 1, 2020 17:31

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Sep 1, 2020

k8s-ci-robot requested review from dashpole, marosset and thockin September 1, 2020 17:41

k8s-ci-robot assigned dchen1107 Sep 1, 2020

dashpole reviewed Sep 1, 2020

View reviewed changes

k8s-ci-robot requested a review from wojtek-t September 1, 2020 18:01

javidiaz force-pushed the fqdn-hostname-kep-update branch from ebc1411 to c5f6e02 Compare September 1, 2020 19:06

k8s-ci-robot changed the title ~~Updating KEP to promote feature to Beta~~ Update FQDN Hostname KEP to beta Sep 2, 2020

wojtek-t self-assigned this Sep 2, 2020

javidiaz mentioned this pull request Sep 13, 2020

Allow users to set a pod’s hostname to its Fully Qualified Domain Name (FQDN) #1797

Closed

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 16, 2020

marosset reviewed Sep 16, 2020

View reviewed changes

keps/sig-node/1797-configure-fqdn-as-hostname-for-pods/kep.yaml Outdated Show resolved Hide resolved

Updating KEP to promote feature

9730f51

javidiaz force-pushed the fqdn-hostname-kep-update branch from 6413d77 to 9730f51 Compare September 17, 2020 14:26

wojtek-t reviewed Sep 18, 2020

View reviewed changes

javidiaz force-pushed the fqdn-hostname-kep-update branch from aa972c7 to 5194498 Compare September 18, 2020 22:02

wojtek-t reviewed Sep 21, 2020

View reviewed changes

javidiaz force-pushed the fqdn-hostname-kep-update branch from 5194498 to 104a792 Compare September 29, 2020 22:47

wojtek-t reviewed Sep 30, 2020

View reviewed changes

Addressing comments

8c78dc7

javidiaz force-pushed the fqdn-hostname-kep-update branch from 104a792 to 8c78dc7 Compare September 30, 2020 13:53

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

javidiaz mentioned this pull request Sep 30, 2020

Adding failure scenario for long FQDN and setHostnameAsFQDN feature kubernetes/kubernetes#95187

Merged

SergeyKanzhelev reviewed Oct 1, 2020

View reviewed changes

keps/sig-node/1797-configure-fqdn-as-hostname-for-pods/README.md Outdated Show resolved Hide resolved

k8s-ci-robot assigned SergeyKanzhelev Oct 1, 2020

Update keps/sig-node/1797-configure-fqdn-as-hostname-for-pods/README.md

80501c8

Co-authored-by: Sergey Kanzhelev <[email protected]>

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2020

wojtek-t reviewed Oct 2, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 2, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2020

k8s-ci-robot merged commit 78ff496 into kubernetes:master Oct 2, 2020

k8s-ci-robot added this to the v1.20 milestone Oct 2, 2020

javidiaz mentioned this pull request Oct 2, 2020

Adding check for readiness approval in FQDN Hostname KEP #2057

Merged

Update FQDN Hostname KEP to beta #1962

Update FQDN Hostname KEP to beta #1962

Uh oh!

Conversation

javidiaz commented Sep 1, 2020

Uh oh!

k8s-ci-robot commented Sep 1, 2020

Uh oh!

javidiaz commented Sep 1, 2020

Uh oh!

javidiaz commented Sep 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danwinship commented Sep 2, 2020

Uh oh!

javidiaz commented Sep 14, 2020

Uh oh!

marosset commented Sep 16, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wojtek-t left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wojtek-t commented Sep 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SergeyKanzhelev commented Oct 1, 2020

Uh oh!

wojtek-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dchen1107 commented Oct 2, 2020