Skip to content
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

🌱 Automatically set kubelet args for capd #8881

Conversation

fabriziopandini
Copy link
Member

@fabriziopandini fabriziopandini commented Jun 19, 2023

What this PR does / why we need it:
This PR uses the kind mapper introduced by #8880, and use it to automatically set some parameters in the node registration, so the kindest/node image are started as expected.

Which issue(s) this PR fixes:
Fixes #

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 19, 2023
@fabriziopandini
Copy link
Member Author

/hold
for #8880

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 19, 2023
@killianmuldoon
Copy link
Contributor

Part of #8788

@sbueringer
Copy link
Member

@fabriziopandini I think this is an old version of the kind mapper PR. Did you maybe lose a commit during rebase?

@fabriziopandini fabriziopandini force-pushed the automatically-set-kubelet-args-for-CAPD branch from aa6e0b4 to fb9055b Compare June 21, 2023 09:40
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 21, 2023
@fabriziopandini fabriziopandini force-pushed the automatically-set-kubelet-args-for-CAPD branch from fb9055b to 61433ec Compare June 22, 2023 14:01
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2023
@fabriziopandini
Copy link
Member Author

/test pull-cluster-api-e2e-full-main

@fabriziopandini
Copy link
Member Author

/hold cancel
@sbueringer @killianmuldoon I also updated E2E templates, CAPD templates, docs
Ignition is now a documented exception where the user has to set manually kubeletExtraArgs id using CAPD; The E2E template is an example of how this can be done

I have grouped changes in different commits for a faster review

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 22, 2023
docs/book/src/tasks/experimental-features/ignition.md Outdated Show resolved Hide resolved
test/e2e/config/docker.yaml Outdated Show resolved Hide resolved
@sbueringer
Copy link
Member

/lgtm
/approve

/hold
for squash

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 23, 2023
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: f657dc1aa2e0e2d65b6b866da5276968358cb021

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2023
docs/book/src/tasks/experimental-features/ignition.md Outdated Show resolved Hide resolved
docs/book/src/tasks/experimental-features/ignition.md Outdated Show resolved Hide resolved
docs/book/src/tasks/experimental-features/ignition.md Outdated Show resolved Hide resolved
if err := yaml.Unmarshal(userData, a); err != nil {
return errors.Wrapf(err, "error parsing write_files action: %s", userData)
}
for i, f := range a.Files {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that adding all this code here, including kindMapping kind.Mapping adds a lot of unnecessary complexity to the solution, as this code has nothing to do with writeFilesAction functionality.

Given that we already have plenty of code duplicated between CloudInit and Ignition code for CAPD, I'd suggest to introduce some intermediate, common format which you can convert both CloudInit and Ignition custom data to to perform CAPD-specific mutations + deduplicate code responsible for converting it into provisioning commands.

Format can be as simple as needed, so something like:

type File struct {
  Path string
  Mode string
  Contents string // Or []byte
}

type ProvisioningData struct {
  Files []File
  Commands []provisioning.Cmd
}

Then you can have a function ProvisioningData -> ProvisioningData, which will apply CAPD specific modifications and []File -> []provisioning.Cmd function to convert all files into creation commands.

This also obliviously enables us to reuse flags modifications between cloud init and Ignition and makes it all very easy to test as it's all data-driven.

Copy link
Member Author

@fabriziopandini fabriziopandini Jun 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finding the best way forward for sharing common code if any is in scope of #8907

This PR scope is to get us in a place where we are less exposed to the impact of changes in the kindest/image, see #8788 for the full context about impact of the latest dump and all the work that followed

@fabriziopandini fabriziopandini force-pushed the automatically-set-kubelet-args-for-CAPD branch from 86c1de9 to ce9291a Compare June 23, 2023 16:13
@fabriziopandini
Copy link
Member Author

squashed commits
running all E2E tests for one time more
/test pull-cluster-api-e2e-full-main

@fabriziopandini
Copy link
Member Author

/test pull-cluster-api-test-main

@killianmuldoon
Copy link
Contributor

This is complex, but actually less complex than the current state of spreading these changes across multiple templates. I think there's probably room for further simplification, but really a good change.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 1dd663dd5f89722192912ba2421d59b9238a0e49

@sbueringer
Copy link
Member

e2e-full failed

re-triggering. Let's definitely wait until it's green before merge

/test pull-cluster-api-e2e-full-main

@sbueringer
Copy link
Member

/test pull-cluster-api-e2e-full-main

to get more data on the test failures

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2023
@fabriziopandini fabriziopandini force-pushed the automatically-set-kubelet-args-for-CAPD branch from d876047 to 090edc2 Compare June 26, 2023 11:54
@fabriziopandini
Copy link
Member Author

/test pull-cluster-api-e2e-full-main

@fabriziopandini fabriziopandini force-pushed the automatically-set-kubelet-args-for-CAPD branch from 090edc2 to 8b15bc0 Compare June 26, 2023 13:52
@fabriziopandini
Copy link
Member Author

/test pull-cluster-api-e2e-full-main

@sbueringer
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 0bc19f1a328076743e0e3342aa04303eca2b252c

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: killianmuldoon, sbueringer

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 [killianmuldoon,sbueringer]

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

@sbueringer
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2023
@k8s-ci-robot k8s-ci-robot merged commit 9a6b99b into kubernetes-sigs:main Jun 26, 2023
11 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.5 milestone Jun 26, 2023
@fabriziopandini fabriziopandini deleted the automatically-set-kubelet-args-for-CAPD branch June 26, 2023 15:55
@killianmuldoon
Copy link
Contributor

/area provider/infrastructure-docker

@k8s-ci-robot k8s-ci-robot added the area/provider/infrastructure-docker Issues or PRs related to the docker infrastructure provider label Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/infrastructure-docker Issues or PRs related to the docker infrastructure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants