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

create: separate bootstrap and node ignition config assets into separate targets #1119

Closed
wants to merge 2 commits into from
Closed

create: separate bootstrap and node ignition config assets into separate targets #1119

wants to merge 2 commits into from

Conversation

staebler
Copy link
Contributor

The Bootstrap Ignition asset is dependent upon the Master and Worker Ignition assets.
It is problematic to have them all targeted by the same sub-command because it makes
it difficult for a user to modify all of the ignition configs. These changes create
a new sub-command that targets just the Master and Worker Ignition assets.

The ignition-configs sub-command has been renamed to pre-cluster. This reflects that
the sub-command targets more than just the Bootstrap Ignition asset, as it targets
the kubeconfig and the metadata.json as well.

The new sub-command has been named node-config.

Fixes https://jira.coreos.com/browse/CORS-948

For discussion on alternative solutions, see https://docs.google.com/document/d/1Wr82knlY5iY5K10GOBk3Q39HabtuoJcU9pFInSKZclA/edit?usp=sharing.

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 23, 2019
staebler added a commit to staebler/hive that referenced this pull request Jan 23, 2019
The openshift-install ignition-configs sub-command has been renamed
to pre-cluster.

See openshift/installer#1119
staebler added a commit to staebler/release that referenced this pull request Jan 23, 2019
The openshift-install ignition-configs sub-command has been renamed to pre-cluster.

See openshift/installer#1119
@crawford
Copy link
Contributor

Can you also re-generate the dependency graph? That will be the ultimate test of whether or not this teases apart the multi-generational relationship.

@staebler
Copy link
Contributor Author

Can you also re-generate the dependency graph? That will be the ultimate test of whether or not this teases apart the multi-generational relationship.

It's already been re-generated.

@crawford
Copy link
Contributor

It's already been re-generated.

Oh, GitHub collapsed the diff and I only saw the one commit. Can you split the SVG generation into separate commit as we've done historically?

@crawford
Copy link
Contributor

/approve

@staebler
Copy link
Contributor Author

It's already been re-generated.

Oh, GitHub collapsed the diff and I only saw the one commit. Can you split the SVG generation into separate commit as we've done historically?

Sure. I didn't realize that the preference was to separate the SVG generation into a separate commit: I've never separated it in any of my past commits.

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crawford, staebler

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

@@ -4,7 +4,8 @@ The installer uses [Semantic Versioning][semver] for its user-facing API.
Covered by the versioning are:

* `openshift-install [options] create install-config`, which will always create `install-config.yaml` in the asset directory, although the version of the generated install-config may change.
* `openshift-install [options] create ignition-configs`, which will always create `bootstrap.ign`, `master.ign`, and `worker.ign` in the asset directory, although the content of the generated files may change.
* `openshift-install [options] create node-config`, which will always create `master.ign` and `worker.ign` in the asset directory, although the content of the generated files may change.
* `openshift-install [options] create pre-cluster`, which will always create `bootstrap.ign` in the asset directory, although the content of the generated files may change.
Copy link
Member

Choose a reason for hiding this comment

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

Call out metadata.json here too? auth/kubeconfig? Hive/BYOI will rely on their presence (CC @dgoodwin, @vrutkovs), but they'll be able to adapt to changes. Do we want to make compat commitments for external tooling that consumes them as well? CC @abhinavdahiya, @crawford.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the change, but I can take it back out if we decide we do not want to make the compatibility commitment.

Use: "node-config",
Short: "Generates the ignition configs for the master and worker nodes",
},
assets: []asset.WritableAsset{&machine.Master{}, &machine.Worker{}},
Copy link
Member

Choose a reason for hiding this comment

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

Actually, who cares about these pointer Ignition configs? Do we even need a target for them? I expect the default pointers will be fine and folks will tweak the machine Ignition configuration by configuring the machine-config server. pre-cluster may be sufficient with it's reduced target set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. I thought that @crawford had a use case for modifying the master and worker ignition configs.

@staebler
Copy link
Contributor Author

staebler commented Feb 4, 2019

/retest

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 5, 2019
@staebler
Copy link
Contributor Author

level=error msg="\t* module.vpc.aws_route_table_association.worker_routing[2]: 1 error occurred:"
level=error msg="\t* aws_route_table_association.worker_routing.2: timeout while waiting for state to become 'success' (timeout: 5m0s)"

/retest

@staebler
Copy link
Contributor Author

Failing tests:

[Conformance][Area:Networking][Feature:Router] The HAProxy router should enable openshift-monitoring to pull metrics [Suite:openshift/conformance/parallel/minimal]

/retest

@staebler
Copy link
Contributor Author

Flaky tests:

[Conformance][Area:Networking][Feature:Router] The HAProxy router should expose prometheus metrics for a route [Suite:openshift/conformance/parallel/minimal]
[Conformance][templates] templateinstance impersonation tests should pass impersonation deletion tests [Suite:openshift/conformance/parallel/minimal]
[Feature:Builds][pullsecret][Conformance] docker build using a pull secret Building from a template should create a docker build that pulls using a secret run it [Suite:openshift/conformance/parallel/minimal]
[k8s.io] InitContainer [NodeConformance] should not start app containers if init containers fail on a RestartAlways pod [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]
[k8s.io] Pods should support pod readiness gates [NodeFeature:PodReadinessGate] [Suite:openshift/conformance/parallel] [Suite:k8s]
[k8s.io] Security Context when creating containers with AllowPrivilegeEscalation should allow privilege escalation when not explicitly set and uid != 0 [NodeConformance] [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-apps] Deployment test Deployment ReplicaSet orphaning and adoption regarding controllerRef [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] In-tree Volumes [Driver: hostPathSymlink] [Testpattern: Inline-volume (default fs)] volumes should be mountable [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-storage] In-tree Volumes [Driver: hostPath] [Testpattern: Inline-volume (default fs)] subPath should support non-existent path [Suite:openshift/conformance/parallel] [Suite:k8s]

Failing tests:

[Conformance][Area:Networking][Feature:Router] The HAProxy router should enable openshift-monitoring to pull metrics [Suite:openshift/conformance/parallel/minimal]
[sig-storage] In-tree Volumes [Driver: aws] [Testpattern: Inline-volume (default fs)] subPath should support readOnly directory specified in the volumeMount [Suite:openshift/conformance/parallel] [Suite:k8s]

/retest

@staebler
Copy link
Contributor Author

staebler commented Feb 15, 2019

Failing tests:

[Conformance][Area:Networking][Feature:Router] The HAProxy router should enable openshift-monitoring to pull metrics [Suite:openshift/conformance/parallel/minimal]

Three times in a row now with this failure. I'll evaluate whether it is a real failure.

@staebler
Copy link
Contributor Author

/retest

@staebler
Copy link
Contributor Author

Failing tests:

[Conformance][Area:Networking][Feature:Router] The HAProxy router should enable openshift-monitoring to pull metrics [Suite:openshift/conformance/parallel/minimal]

/retest

@staebler
Copy link
Contributor Author

/retest

…ate targets

The Bootstrap Ignition asset is dependent upon the Master and Worker Ignition assets.
It is problematic to have them all targeted by the same sub-command because it makes
it difficult for a user to modify all of the ignition configs. These changes create
a new sub-command that targets just the Master and Worker Ignition assets.

The ignition-configs sub-command has been renamed to pre-cluster. This reflects that
the sub-command targets more than just the Bootstrap Ignition asset, as it targets
the kubeconfig and the metadata.json as well.

The new sub-command has been named node-config.

The ignitions-configs sub-command has been deprecated and will be rmeoved once it is
no longer used.

Fixes https://jira.coreos.com/browse/CORS-948
The ignition-config was split into node-config and pre-cluster targets
so the dependency graph needed to be updated.

Generated with:

$ openshift-install graph | dot -Tsvg >docs/design/resource_dep.svg

using:

$ dot -V
dot - graphviz version 2.40.1 (20161225.0304)
@openshift-ci-robot
Copy link
Contributor

@staebler: The following test failed for commit 69b980d, say /retest to rerun them:

Test name Details Rerun command
ci/prow/e2e-aws link /test e2e-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@staebler
Copy link
Contributor Author

/hold

Hold for 4.1.

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 25, 2019
@openshift-ci-robot
Copy link
Contributor

@staebler: PR needs rebase.

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.

@staebler
Copy link
Contributor Author

/close

Superseded by #1532.

@openshift-ci-robot
Copy link
Contributor

@staebler: Closed this PR.

In response to this:

/close

Superseded by #1532.

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.

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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants