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

Update ClusterOperator API and fix name #71

Conversation

Miciah
Copy link
Contributor

@Miciah Miciah commented Feb 4, 2019

Update openshift/api dependency

  • Gopkg.lock:
  • Gopkg.toml:
  • vendor/github.com/openshift/api: Update package in order to have the necessary API version to set operand versions in ClusterOperator status.

Set ClusterOperator status.versions and status.relatedObjects

Update to the ClusterOperator definition in the latest openshift/api, which replaces the .status.version field with a .status.versions field and adds a .status.relatedObjects field.

This commit resolves NE-144.

  • cmd/cluster-dns-operator/main.go (main): Set Config in Handler.
  • pkg/stub/handler.go (Handler): Add Config field.
  • pkg/stub/status.go (syncOperatorStatus): Set status.versions and status.relatedObjects.
  • pkg/util/clusteroperator/status.go (ObjectReferencesEqual): New function to help test whether the related objects changed in the status.
    (VersionsEqual): New function to help test whether the operand versions changed in the status.
  • pkg/util/clusteroperator/status_test.go (TestObjectReferencesEqual):
    (TestVersionsEqual): New tests.

Change the ClusterOperator object's name

Change the name of the ClusterOperator object from "openshift-dns-operator" to "dns".

  • pkg/stub/status.go (syncOperatorStatus): Change the ClusterOperator name from "openshift-dns-operator" to "dns".
  • test/e2e/operator_test.go (TestOperatorAvailable): Adjust to the new name for the ClusterOperator.

* Gopkg.lock:
* Gopkg.toml:
* vendor/github.com/openshift/api: Update package in order to have the
necessary API version to set operand versions in ClusterOperator status.
@Miciah Miciah force-pushed the NE-144-update-ClusterOperator-API-and-fix-name branch from 8d20b8b to 7ae8982 Compare February 4, 2019 03:05
@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 4, 2019
@Miciah
Copy link
Contributor Author

Miciah commented Feb 4, 2019

cluster-operators.json from the e2e-aws-operator test has the following:

        {
            "apiVersion": "config.openshift.io/v1",
            "kind": "ClusterOperator",
            "metadata": {
                "creationTimestamp": "2019-02-04T04:56:46Z",
                "generation": 1,
                "name": "dns",
                "resourceVersion": "1480",
                "selfLink": "/apis/config.openshift.io/v1/clusteroperators/dns",
                "uid": "4643af95-2839-11e9-b790-126a6381799a"
            },
            "spec": {},
            "status": {
                "conditions": [
                    {
                        "lastTransitionTime": "2019-02-04T04:56:46Z",
                        "status": "False",
                        "type": "Failing"
                    },
                    {
                        "lastTransitionTime": "2019-02-04T04:56:46Z",
                        "status": "False",
                        "type": "Progressing"
                    },
                    {
                        "lastTransitionTime": "2019-02-04T04:57:17Z",
                        "status": "True",
                        "type": "Available"
                    }
                ],
                "extension": null,
                "relatedObjects": [
                    {
                        "group": "",
                        "name": "openshift-dns-operator",
                        "resource": "namespaces"
                    },
                    {
                        "group": "",
                        "name": "openshift-dns",
                        "resource": "namespaces"
                    }
                ],
                "versions": [
                    {
                        "name": "operator",
                        "version": "0.0.1"
                    },
                    {
                        "name": "coredns",
                        "version": "registry.svc.ci.openshift.org/ci-op-slzhbpgm/stable@sha256:e4936a702d7d466a64a6a9359f35c7ad528bba7c35fe5c582a90e46f9051d8b8"
                    }
                ]
            }
        },

Edit: Updated with link and excerpt from the last test run.

Copy link

@pravisankar pravisankar left a comment

Choose a reason for hiding this comment

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

Added couple of comments, rest of the changes LGTM.

oldVersions := co.Status.Versions
coreDNSVersion := h.Config.CoreDNSImage
if versionIdx := strings.Index(coreDNSVersion, ":"); versionIdx >= 0 {
coreDNSVersion = coreDNSVersion[versionIdx+1:]

Choose a reason for hiding this comment

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

Instead of extracting the coredns revision, do we want to use the coredns imageID ?
(Similar to https://github.com/openshift/cluster-version-operator/pull/104/files#diff-437d518ae5ea84d0c4284dd06bfe3fd7R79 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would simply using h.Config.CoreDNSImage be reasonable? I don't think the operator should have to figure out the image's sha256.

Choose a reason for hiding this comment

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

Will that be similar to 'docker.io/openshift/origin-coredns:v4.0' (it may have openshift registry prefix instead of docker.io)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the last test run, it was registry.svc.ci.openshift.org/ci-op-slzhbpgm/stable@sha256:e4936a702d7d466a64a6a9359f35c7ad528bba7c35fe5c582a90e46f9051d8b8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking again at https://github.com/openshift/cluster-version-operator/pull/104/files#diff-437d518ae5ea84d0c4284dd06bfe3fd7R79, maybe registry.svc.ci.openshift.org/ci-op-slzhbpgm/stable@sha256:e4936a702d7d466a64a6a9359f35c7ad528bba7c35fe5c582a90e46f9051d8b8. is the expected value (I don't understand what "substitution values" are or what should substitute them).

Choose a reason for hiding this comment

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

If we use this as coredns version, how can we decrypt which version of coredns is used by user?
It does not look like e4936a702d7d466a6... maps to openshift/coredns commit revision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm pretty sure that's a checksum of the container image, and I don't know how to translate it back to a Git commit or other meaningful identifier.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's some oc tooling to find all this info like oc adm release info --pullspecs and oc image info... probably more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying we could look at said tooling to see how the operator can find this info, or are you saying we are providing sufficient info for a human to use the tooling to find the needed info?

Copy link
Contributor

Choose a reason for hiding this comment

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

For now the latter. Let's report the thing we're actually deploying and if other details need to be derived from it, approach separately.

pkg/stub/status.go Show resolved Hide resolved
Update to the ClusterOperator definition in the latest openshift/api, which
replaces the .status.version field with a .status.versions field and adds a
.status.relatedObjects field.

This commit resolves NE-144.

https://jira.coreos.com/browse/NE-144

* cmd/cluster-dns-operator/main.go (main): Set Config in Handler.
* pkg/stub/handler.go (Handler): Add Config field.
* pkg/stub/status.go (syncOperatorStatus): Set .status.versions and
.status.relatedObjects.
* pkg/util/clusteroperator/status.go (ObjectReferencesEqual): New function
to help test whether the related objects changed in the status.
(VersionsEqual): New function to help test whether the operand versions
changed in the status.
* pkg/util/clusteroperator/status_test.go (TestObjectReferencesEqual):
(TestVersionsEqual): New tests.
Change the name of the ClusterOperator object from "openshift-dns-operator"
to "dns".

* pkg/stub/status.go (syncOperatorStatus): Change the ClusterOperator name
from "openshift-dns-operator" to "dns".
* test/e2e/operator_test.go (TestOperatorAvailable): Adjust to the new name
for the ClusterOperator.
@Miciah Miciah force-pushed the NE-144-update-ClusterOperator-API-and-fix-name branch from 7ae8982 to 1904808 Compare February 4, 2019 04:43
@ironcladlou
Copy link
Contributor

Let's get it going and fix bugs as necessary.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 5, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, Miciah

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-merge-robot openshift-merge-robot merged commit fb24eeb into openshift:master Feb 5, 2019
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.

None yet

5 participants