dep: bump to kubernetes 1.16 #301

ironcladlou · 2019-09-24T13:44:39Z

Depends on kubernetes-sigs/controller-runtime#588.

TODO

Verify correctness of the regenerated CRD manifests

sttts · 2019-09-24T15:31:35Z

manifests/00-custom-resource-definition.yaml

@@ -442,22 +88,23 @@ spec:
                hostNetwork:
                  description: hostNetwork holds parameters for the HostNetwork endpoint
                    publishing strategy. Present only if type is HostNetwork.
+                  nullable: true


this makes the schema weaker. Probably you have a // +kubebuilder:nullable=true tag or so in the API types?

Fixed in openshift/api#438

Fixed here.

sttts · 2019-09-24T15:31:47Z

manifests/00-custom-resource-definition.yaml

                  properties:
                    scope:
                      description: scope indicates the scope at which the load balancer
                        is exposed. Possible values are "External" and "Internal".  The
                        default is "External".
                      type: string
-                  required:
-                  - scope


this looks suspicious.

4.1 didn't have scope, so it cannot be required.

It's required in the 4.2 branch... https://github.com/openshift/cluster-ingress-operator/blob/release-4.2/manifests/00-custom-resource-definition.yaml#L455

@Miciah https://github.com/openshift/cluster-ingress-operator/blob/release-4.2/manifests/00-custom-resource-definition.yaml#L455 does this mean we need to change 4.2 before release? Is there a problem that has gone unnoticed?

Seems like this would be a problem. I can understand how we would have missed this for spec.endpointPublishingStrategy.scope: spec.endpointPublishingStrategy is optional, and the operator-created default ingresscontroller does not specify it. However, I wonder why we have not seen problems with status.endpointPublishingStrategy.scope during upgrade tests. I'll launch a 4.1 cluster, set spec.endpointPublishingStrategy with null spec.endpointPublishingStrategy.scope, upgrade the cluster to 4.2, and report back what happens.

@Miciah based on your upgrade testing, seems like this is going to be okay?

Right, the changing required value did not seem to have any effect: I performed the steps I laid out in my previous comment and saw no errors in the operator logs or command output when the upgraded ingress operator reconciled the old IngressController resource that had spec.endpointPublishingStrategy but not spec.endpointPublishingStrategy.scope set and when I tried to update the resource using oc annotate.

Facepalm: endpointPublishingStrategy.loadBalancer is optional, so endpointPublishingStrategy.loadBalancer.scope can be required. I erroneously wrote endpointPublishingStrategy.scope instead of endpointPublishingStrategy.loadBalancer.scope earlier. I think 4.2 is fine to make scope required, and we should keep it required in 4.3.

openshift/api#446 will decide the fate of this field

sttts · 2019-09-24T15:31:59Z

manifests/00-custom-resource-definition.yaml

@@ -481,8 +128,6 @@ spec:
                    networking, and is not explicitly published. The user must manually
                    publish the ingress controller."
                  type: string
-              required:
-              - type


this looks suspicious

I added +optional when I added +unionDiscriminator in openshift/api@2024105#diff-85d84174432a6d2df99d107129e012f8R246, but maybe I shouldn't have. https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/20190325-unions.md#go-tags says, "// +unionDiscriminator before a field means that this field is the discriminator for the union. Only one field per structure can have this prefix. This field HAS TO be a string, and CAN be optional."

@Miciah what do you propose we do here?

I do not know of a reason why this field should be marked as optional (it came up during review: openshift/api#312 (comment), but I do not see a reason given why it should be optional). spec.endpointPublishingStrategy is optional, so I believe type does not need to be so. If it is not too late to do so, I would like to remove +optional. The question that was raised in the discussion about the +nullable markers applies here: Since the old generated CRD does not reflect the markers, is it safe to remove them?

Like with nullable, how about open a PR and let's get the discussion centralized?

openshift/api#440

sttts · 2019-09-24T15:32:11Z

manifests/00-custom-resource-definition.yaml

@@ -705,9 +361,6 @@ spec:
                    type: string
                  type:
                    type: string
-                required:
-                - type
-                - status


this looks suspicious.

@sttts Isn't this from https://github.com/openshift/api/blob/master/operator/v1/types.go#L148? They aren't omit empty and aren't explicitly marked required (the package default is optional). Is something wrong with the shared OperatorCondition struct's markers?

Discussion in openshift/api#445

sttts · 2019-09-24T15:32:20Z

manifests/00-custom-resource-definition.yaml

                  properties:
                    scope:
                      description: scope indicates the scope at which the load balancer
                        is exposed. Possible values are "External" and "Internal".  The
                        default is "External".
                      type: string
-                  required:
-                  - scope


suspicious.

Status mirrored from spec... should it follow the same validation?

Seems like this should be optional and the requiredness was possibly a mistake. See #301 (comment).

@Miciah based on your upgrade testing, seems like this is going to be okay?

ironcladlou · 2019-09-25T18:08:27Z

@Miciah updated to incorporate openshift/api#440 and openshift/api#438, PTAL

ironcladlou · 2019-09-25T21:06:33Z

Waiting on openshift/library-go#531 to pick up openshift/api#442.

ironcladlou · 2019-09-25T21:45:54Z

manifests/00-custom-resource-definition.yaml

-          required:
-          - availableReplicas
-          - selector
-          - domain


@Miciah hmmm

This is in status, so I don't know that we care...?

I don't know how to think about this one. Maybe @deads2k can help or knows who can.

ironcladlou · 2019-09-30T13:44:07Z

@Miciah I think I got this all cleaned up with the exception of openshift/api#445 which we can address in a followup as necessary. PTAL.

Miciah · 2019-09-30T15:05:33Z

We need to sort out openshift/api#445 as well as #301 (comment) (concerning status.availableReplicas, status.selector, and status.domain), but we can get those in a follow-up.
/lgtm

openshift-ci-robot · 2019-09-30T15:05:52Z

[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:

~~OWNERS~~ [Miciah,ironcladlou]

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

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 24, 2019

openshift-ci-robot requested review from knobunc and Miciah September 24, 2019 13:44

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 24, 2019

sttts reviewed Sep 24, 2019

View reviewed changes

ironcladlou changed the title ~~WIP: dep: bump to kubernetes 1.16~~ dep: bump to kubernetes 1.16 Sep 24, 2019

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 24, 2019

ironcladlou force-pushed the rebase-1.16 branch from 893677b to cdc36f9 Compare September 25, 2019 18:07

ironcladlou force-pushed the rebase-1.16 branch from cdc36f9 to 3e34532 Compare September 25, 2019 21:40

ironcladlou commented Sep 25, 2019

View reviewed changes

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 25, 2019

This was referenced Sep 26, 2019

operator: add missing markers to condition type openshift/api#445

Closed

NE-226: Support both DNSZone and PrivateDNSZone resources #300

Merged

ironcladlou added 3 commits September 30, 2019 09:41

dep: bump to kubernetes 1.16

a79c29b

dep: bump openshift/api and openshift/library-go

8eb04c2

manifests: regenerate

1e17863

ironcladlou force-pushed the rebase-1.16 branch from 3e34532 to 1e17863 Compare September 30, 2019 13:42

openshift-ci-robot assigned Miciah Sep 30, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2019

openshift-merge-robot merged commit 14b8b8d into openshift:master Sep 30, 2019

ironcladlou mentioned this pull request Oct 2, 2019

Update to Kube 1.16 openshift/cluster-dns-operator#131

Merged

dep: bump to kubernetes 1.16 #301

dep: bump to kubernetes 1.16 #301

Conversation

ironcladlou commented Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miciah Sep 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ironcladlou commented Sep 25, 2019

ironcladlou commented Sep 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ironcladlou commented Sep 30, 2019

Miciah commented Sep 30, 2019 • edited Loading

openshift-ci-robot commented Sep 30, 2019

ironcladlou commented Sep 24, 2019 •

edited

Loading

Miciah Sep 26, 2019 •

edited

Loading

Miciah commented Sep 30, 2019 •

edited

Loading