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

Remove clusterID from installconfig and move it to cluster #1057

Merged
merged 8 commits into from
Jan 12, 2019
Merged

Remove clusterID from installconfig and move it to cluster #1057

merged 8 commits into from
Jan 12, 2019

Conversation

staebler
Copy link
Contributor

#783 rebased on top of #1052

staebler and others added 8 commits January 11, 2019 09:36
None of the options, other than Image, that were in the Libvirt MachinePool were being used.
They have been removed. The Image has been pulled up to the Libvirt Platform, as there was no
way to use a different image for different machines pools.

For consistency with the AWS and OpenStack platforms, the Libvirt MachinePool has been retained,
even though it is empty. The DefaultMachinePlatform has been retained in the Libvirt Platform
as well.

The code in the Master Machines and Worker Machines assets that determines the configuration
to use for the machines has been adjusted for Libvirt to rectify the machine-pool-specific
configuration against the default machine-pool configuration. This is not strictly necessary
as, again, the Libvirt configuration is empty. It keeps the logic consistent with the other
platforms, though.

https://jira.coreos.com/browse/CORS-911
added OPENSHIFT_INSTALL_OS_IMAGE_OVERRIDE env with warning to override the image that will be used.
With baseimage user query removed from TUI, this function is no longer required.

This also
- drops the vendored files
- updates the mocks using `hack/go-genmock.sh`
RHCOS image that needs to be used for installation must be sources from RHCOS build pipeline and RHCOS build.
Keeping the image related fields in install-config allows users to change these values as part of valid configuration, but we do not want users to configure this option as
the RHCOS image controls the runtime and kubelet versions we depend on.
ClusterID is now removed from installconfig. The reason is that it should not be possible for a user to override this value. The clusterID is still needed for destroy - and hence it is now a separate asset which gets stored in ClusterMetadata. Other assets needing the clusterID (e.g. legacy manifests) issue a separate dependency on this asset.
For convenience of package depenencies, the asset still lives in the installconfig package.
@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 12, 2019
@wking
Copy link
Member

wking commented Jan 12, 2019

/lgtm
/hold

I'll pull the /hold once #1052 lands.

@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. lgtm Indicates that a PR is ready to be merged. labels Jan 12, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@wking
Copy link
Member

wking commented Jan 12, 2019

#1052 landed.

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 12, 2019
@wking
Copy link
Member

wking commented Jan 12, 2019

e2e-aws:

error: unable to connect to image repository registry.svc.ci.openshift.org/ci-op-n9v1pkj1/stable@sha256:6cd9842c7897cff8eb05d6e3a9b9dd05de21ae484a68d2b7dba8d71e58b7c22d: Get https://registry.svc.ci.openshift.org/v2/: net/http: TLS handshake timeout

/retest

@abhinavdahiya
Copy link
Contributor

Failing tests:

[Conformance][Area:Networking][Feature:Router] The HAProxy router converges when multiple routers are writing conflicting status [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] [sig-node] Security Context [Feature:SecurityContext] should support seccomp default which is unconfined [Feature:Seccomp] [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-api-machinery] Servers with support for API chunking should return chunks of results for list calls [Suite:openshift/conformance/parallel] [Suite:k8s]
[sig-apps] StatefulSet [k8s.io] Basic StatefulSet functionality [StatefulSetBasic] Burst scaling should run to completion even with unhealthy pods [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]
[sig-storage] Projected should set mode on item file [NodeConformance] [Conformance] [Suite:openshift/conformance/parallel/minimal] [Suite:k8s]

Writing JUnit report to /tmp/artifacts/junit/junit_e2e_20190112-072844.xml

Error: 6 fail, 519 pass, 97 skip (28m54s)

/retest

@openshift-merge-robot openshift-merge-robot merged commit 55f3d9f into openshift:master Jan 12, 2019
@dgoodwin
Copy link
Contributor

Per last arch call Hive and service delivery both have a need to know the cluster ID, before we start provisioning resources. With this change it looks like we can no longer specify our own, but I'm not seeing the cluster metadata on disk when I generate manifests. Did this change make it in before this? Any advice on what our best path here is to know that UUID before we provision? I do see a few mentions of it in state file and cvo-overrides.yml after generating manifests.

CC @crawford @wking

@crawford
Copy link
Contributor

@dgoodwin oops, I think this was merged a little prematurely. We'll need to follow up with an easy way to capture that UUID before the provisioning is started.

CC @abhinavdahiya

@wking
Copy link
Member

wking commented Jan 14, 2019

We'll need to follow up with an easy way to capture that UUID before the provisioning is started.

Instead of just the UUID, can we make a metadata.json asset that is part of the ignition-configs target?

@crawford
Copy link
Contributor

@dgoodwin Abhinav pointed out that metadata.json will always be written out after the cluster has finished creation (regardless of whether or not it was successful). Is this sufficient? Is the cluster ID needed before we start provisioning?

@wking
Copy link
Member

wking commented Jan 14, 2019

You need metadata.json before create cluster because bring-your-own-infrastructure will never actually call create cluster, and they'll still want the metadata to be able to destroy clusters later.

@wking
Copy link
Member

wking commented Jan 15, 2019

You need metadata.json before create cluster because bring-your-own-infrastructure...

Never mind, I was confused. Bring-your-own-infrastructure must also be destroy-your-own-infrastructure. For Hive or anything that is actually calling create cluster, I agree that "metadata.json will be there when we exit" seems to cover things. @dgoodwin, can you remind us why you wanted it before calling create cluster? I've pushed up an implementation here if it turns out we do need this, but realized I had misunderstood the motivation when writing up the commit message.

@dgoodwin
Copy link
Contributor

The two uses cases were (1) service delivery will start receiving telemetry for the cluster while it's installing, but they have no knowledge of the UUID which is a problem for them, and (2) if Hive fails to upload that UUID after install we have an orphaned cluster that can't be cleaned up automatically. Writing the metadata.json as an asset is a perfect solution, we can upload once ready and if it fails, no harm done, we'll just keep retrying.

wking added a commit to wking/openshift-installer that referenced this pull request Jan 16, 2019
From Devan Goodwin [1]:

  The two uses cases were (1) service delivery will start receiving
  telemetry for the cluster while it's installing, but they have no
  knowledge of the UUID which is a problem for them, and (2) if Hive
  fails to upload that UUID after install we have an orphaned cluster
  that can't be cleaned up automatically.  Writing the metadata.json
  as an asset is a perfect solution, we can upload once ready and if
  it fails, no harm done, we'll just keep retrying.

Matthew recommended the no-op load [2]:

  My suggestion is that, for now, Load should return false always.
  The installer will ignore any changes to metadata.json.  In the
  future, perhaps we should introduce a read-only asset that would
  cause the installer to warn (or fail) in the face of changes.

[1]: openshift#1057 (comment)
[2]: openshift#1070 (comment)
markmc added a commit to markmc/dev-scripts that referenced this pull request Feb 20, 2019
Since 0.10.0, clusterID is no longer part of install-config.yaml

See openshift/installer#1057
hardys pushed a commit to openshift-metal3/dev-scripts that referenced this pull request Feb 20, 2019
Since 0.10.0, clusterID is no longer part of install-config.yaml

See openshift/installer#1057
wking added a commit to wking/openshift-release that referenced this pull request Apr 1, 2019
Catching up with openshift/installer@170fdc2d2c (pkg/asset/: Remove
clusterID from installconfig and move it to cluster, 2018-12-04,
openshift/installer#1057).
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. lgtm Indicates that a PR is ready to be merged. 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.

7 participants