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

Pull layering/ changes to go modules #3126

Closed
wants to merge 3 commits into from

Conversation

cgwalters
Copy link
Member

There's a big pile of conflicts on the go modules go.mod and vendor/ etc. between master and layering. I've cherry picked the bits we need, which should help us keep the layering branch up to date.

IMPORTANT - this PR cherry picks coreos/butane#341 - so that change will currently need to be manually reapplied after any go mod vendor until it merges. This replaces the not-going-to-be upstream Ignition patch.


Update vendoring github.com/coreos/fcct → github.com/coreos/butane

Prep for using Butane APIs more directly as part of the layering
work.

The logic is also a bit reworked to generate a single Butane fragment
which we convert to Ignition in one go, instead of converting
butane into ignition repeatedly and using config merging.

There's only one wrinkle with doing this, which is that today in
the templates we have multiple files which are drop-ins for crio.service;
and we need to group those together.

(I think it would be cleaner to have them in a single file in the
templates, but for clarity let's handle this)


vendor: Cherry pick butane patch for compression

See coreos/butane#341

This obviates coreos/ignition#1329
which we are carrying currently.


make go-deps && make update

A lot of this is version bumps, but go-containerregistry (the thing I used to create my tiny machineconfig wrapper images without a docker build) additionally drags in the following dependencies that nothing else uses:

  • github.com/spf13/viper/
  • github.com/containerd/stargz-snapshotter/
  • github.com/docker/cli/
  • github.com/docker/docker/pkg/homedir/

This isn't terrible, so I'm leaving it for now since it works, but
at some point this might get obviated by buildah or something


@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 29, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 29, 2022
@cgwalters
Copy link
Member Author

utils.go:166: Pool node-max-pods has rendered configs [99-node-max-pods-generated-kubelet] with rendered-node-max-pods-e975347c668037079d4ad843dd29250e (waited 2.07656008s)

Hmm...butane fix didn't work. I think it's because we're not actually using butane config merging, we're doing Ignition config merging. Clearly we need a unit test so this doesn't just fail in e2es which are much more expensive to run.

@cgwalters
Copy link
Member Author

OK I rebased this on #3128 - let's get that one in first.

Prep for using Butane APIs more directly as part of the layering
work.

The logic is also a bit reworked to generate a single Butane fragment
which we convert to Ignition in one go, instead of converting
butane into ignition repeatedly and using config merging.

There's only one wrinkle with doing this, which is that today in
the templates we have multiple files which are drop-ins for `crio.service`;
and we need to group those together.

(I think it would be cleaner to have them in a single file in the
 templates, but for clarity let's handle this)
@cgwalters
Copy link
Member Author

OK cool, #3128 made it in, so rebased 🏄 and I also snuck in one small further fix.

I don't know why I copy/pasted instead originally instead of just
having this call `NewIgnFileBytes()`.
@cgwalters
Copy link
Member Author

Eh actually of course go will remove unused dependencies so I can't quite cherry pick the layering deps right now.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 3, 2022

@cgwalters: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gcp-op 8dce9a8 link true /test e2e-gcp-op

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@cgwalters
Copy link
Member Author

Will revisit this separately

@cgwalters cgwalters closed this May 5, 2022
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. layering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant