new CR: podnetworkconnectivitychecks.controlplane.operator.openshift.io/v1alpha1 #639

sanchezl · 2020-05-04T14:48:06Z

Define CRD in support of openshift/enhancements#289.

sttts · 2020-05-05T14:41:44Z

hack/lib/init.sh

@@ -23,6 +23,7 @@ network/v1 \
 oauth/v1 \
 openshiftcontrolplane/v1 \
 operator/v1 \
+operatorcontrolplane/v1alpha1 \


what is the rational of this group name?

See openshift/enhancements#289 (comment)

operator.openshift.io as suggested by @deads2k is fine. Who says this will only be used in control plane operator?

sttts · 2020-05-05T14:44:20Z

operatorcontrolplane/v1alpha1/0000_10-pod-network-connectivity-check.crd.yaml

@@ -0,0 +1,163 @@
+apiVersion: apiextensions.k8s.io/v1beta1


Don't we support v1 now?

I did try it initially, but the crd codegen seemed to ignore it and insisted on creating the v1beta1 version instead. I can look into it later (plus i would like to clean up those scripts anyway)

@damemi this should work, shouldn't it?

which codegen are you using, controller-gen? Our CRD gen doesn't touch this setting that I'm aware of (just the openapi spec) so I'm wondering what tool could be causing this

controller-gen has a crdVersions flag, which defaults to v1beta1: https://github.com/kubernetes-sigs/controller-tools/blob/master/pkg/crd/gen.go#L63-L71

@sttts, @damemi
The version of controller-gen we are using (github.com/openshift/kubernetes-sigs-controller-tools v0.2.1-37-ga3cca5d) seems to be hard coded to use v1beta1 when generating CRDs.
https://github.com/openshift/kubernetes-sigs-controller-tools/blob/a3cca5d66f230ea5ca3554aef7e130750b841825/pkg/crd/spec.go#L25

@sanchezl I see. No blocker here. @damemi we should rebase controller-gen

sttts · 2020-05-05T14:46:18Z

operatorcontrolplane/v1alpha1/types_conditioncheck.go

+	// be resolved. Specify an IP address for host to bypass DNS name lookup.
+	// +kubebuilder:validation:Required
+	// +required
+	TargetEndpoint string `json:"targetEndpoint"`


set the format, compare kubebuilder:validation:Format and https://github.com/kubernetes/apiextensions-apiserver/blob/master/pkg/apis/apiextensions/v1/types_jsonschema.go#L33

I didn't see an appropriate format (e.g. host:port), so I will use a pattern instead:

// +kubebuilder:validation:Pattern=^\S+:\d*$

(one or more non-whitespace + ':' + port number )

am not fluent in regexes enough: \S includes numbers and other chars allowed in hosts?

\S is non-whitespace. So the regex means anything-non-whitespace, colon, digits.

@sttts The regex isn't perfect, I started to write a more thorough one, but didn't see the point if it wasn't going to be perfect as it would have to handle hostnames, ipv4, ipv6 (which has :, and %, and brackets, etc..) simultaneously.

sttts · 2020-05-05T14:46:34Z

operatorcontrolplane/v1alpha1/types_conditioncheck.go

+	// SourcePod names the pod from which the condition will be checked
+	// +kubebuilder:validation:Required
+	// +required
+	SourcePod string `json:"sourcePod"`


set the allowed values

Added a Pattern.

I believe you have to put ^$ around

sttts · 2020-05-05T14:47:09Z

operatorcontrolplane/v1alpha1/types_conditioncheck.go

+	// SourcePod names the pod from which the condition will be checked
+	// +kubebuilder:validation:Required
+	// +required
+	SourcePod string `json:"sourcePod"`


This is really just documentation, isn't it? It won't magically add another tester sidecar. Would document that here.

The sidecar is added by the operator.

The PodNetworkConnectivityCheck resources also created by operator: one per pod, per endpoint.

The sidecar uses this field to find the endpoints it needs to test, and updates the status with results.

This is really just documentation, isn't it? It won't magically add another tester sidecar. Would document that here.

the container looks up it's own pod name to figure out what to try to contact

oh, ok. So in theory one could change that. Just a new object won't have any effect without a sidecar being deployed for a matching pod.

sttts · 2020-05-05T14:47:38Z

operatorcontrolplane/v1alpha1/types_conditioncheck.go

+type PodNetworkConnectivityCheckStatus struct {
+	// Successes contains logs successful check actions
+	// +optional
+	Successes []LogEntry `json:"successes"`


the last n? who defines n?

the last n? who defines n?

the binary itself. let's say 10 in that binary to start. We do similar sorts of things for our operator revisions.

sttts · 2020-05-05T14:48:23Z

operatorcontrolplane/v1alpha1/types_conditioncheck.go

+// LogEntry records events
+type LogEntry struct {
+	// Start time of check action.
+	Start metav1.Time `json:"time,omitempty"`


why is this not required?

Same as https://github.com/openshift/api/pull/639/files#r420168738, nullable

sttts · 2020-05-05T14:48:59Z

operatorcontrolplane/v1alpha1/types_conditioncheck.go

+
+	// Latency records how long the action mentioned in the entry took.
+	// +optional
+	Latency metav1.Duration `json:"latency"`


this must be nullable because Duration has a custom marshaller.

sttts · 2020-05-05T14:49:39Z

operatorcontrolplane/v1alpha1/types_conditioncheck.go

+
+	// End of outage detected
+	// +kubebuilder:validation:Required
+	// +optional


optional or required?

optional or required?

optional, not every outage has ended.

sttts · 2020-05-05T14:50:06Z

operatorcontrolplane/v1alpha1/types_conditioncheck.go

+	DNSResolve      LogEntryReason = "DNSResolve"
+	DNSError        LogEntryReason = "DNSError"
+	TCPConnect      LogEntryReason = "TCPConnect"
+	TCPConnectError LogEntryReason = "TCPConnectError"


Error is not always timeout, for example no route to host.

I think I wanted to say that I miss a timeout value.

sttts · 2020-05-11T15:23:05Z

operatorcontrolplane/v1alpha1/types_conditioncheck.go

+
+	// Status contains the observed status of the connectivity check
+	// +optional
+	Status PodNetworkConnectivityCheckStatus `json:"status,omitempty"`


omitempty has no effect on non-pointers.

sttts · 2020-05-18T14:50:11Z

/approve
/lgtm

sttts · 2020-05-18T14:50:56Z

operatorcontrolplane/v1alpha1/doc.go

+
+// +kubebuilder:validation:Optional
+// +groupName=controlplane.operator.openshift.io
+package v1alpha1


fix the PR description to alpha.

Is this fixed?

@sttts done

sanchezl · 2020-06-05T14:16:09Z

/retest

openshift-bot · 2020-06-05T14:28:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

deads2k · 2020-06-05T22:56:22Z

operatorcontrolplane/v1alpha1/types_conditioncheck.go

+	// Success indicates if the log entry indicates a success or failure.
+	// +kubebuilder:validation:Required
+	// +required
+	Success bool `json:"success"`


followup: string. nothing is a bool.

deads2k · 2020-06-05T22:57:23Z

/lgtm

openshift-ci-robot · 2020-06-05T22:57:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, sanchezl, sttts

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~~ [deads2k,sttts]

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 requested review from abhinavdahiya and adambkaplan May 4, 2020 14:48

sanchezl mentioned this pull request May 4, 2020

stability: point-to-point network check prototype openshift/enhancements#304

Closed

6 tasks

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels May 4, 2020

sanchezl changed the title ~~Add PodNetworkConnectivityCheck CRD~~ WIP: add PodNetworkConnectivityCheck CRD May 4, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 4, 2020

sanchezl marked this pull request as draft May 4, 2020 18:42

sanchezl changed the title ~~WIP: add PodNetworkConnectivityCheck CRD~~ add PodNetworkConnectivityCheck CRD May 4, 2020

sanchezl marked this pull request as ready for review May 4, 2020 18:46

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 May 4, 2020

sanchezl marked this pull request as draft May 4, 2020 18:46

sttts reviewed May 5, 2020

View reviewed changes

sanchezl force-pushed the point-to-point branch 4 times, most recently from 4eaed47 to 3214cbb Compare May 8, 2020 19:40

sttts reviewed May 11, 2020

View reviewed changes

sanchezl force-pushed the point-to-point branch 3 times, most recently from ba05d83 to 7f5b916 Compare May 15, 2020 01:56

sanchezl force-pushed the point-to-point branch 2 times, most recently from 4841bc9 to c4f5eb1 Compare May 18, 2020 14:44

sanchezl marked this pull request as ready for review May 18, 2020 14:46

openshift-ci-robot assigned sttts May 18, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 18, 2020

sttts reviewed May 18, 2020

View reviewed changes

sanchezl changed the title ~~add PodNetworkConnectivityCheck CRD~~ add PodNetworkConnectivityCheck/v1alpha1 CRD May 18, 2020

sanchezl changed the title ~~add PodNetworkConnectivityCheck/v1alpha1 CRD~~ add CR (Alpha): podnetworkconnectivitychecks.controlplane.operator.openshift.io/v1alpha1 May 29, 2020

sanchezl changed the title ~~add CR (Alpha): podnetworkconnectivitychecks.controlplane.operator.openshift.io/v1alpha1~~ new CR: podnetworkconnectivitychecks.controlplane.operator.openshift.io/v1alpha1 May 29, 2020

sanchezl added 3 commits June 5, 2020 10:27

add operatorcontrolplane/v1 group

eb3573d

enable generation for operatorcontrolplane/v1 group

27d67b6

generate operatorcontrolplane/v1 group artifacts

0cc9aa6

sanchezl force-pushed the point-to-point branch from c4f5eb1 to 0cc9aa6 Compare June 5, 2020 14:33

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2020

deads2k reviewed Jun 5, 2020

View reviewed changes

openshift-ci-robot assigned deads2k Jun 5, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2020

openshift-merge-robot merged commit fb2a6ca into openshift:master Jun 5, 2020

sanchezl mentioned this pull request Jun 6, 2020

followup: string. nothing is a bool. #657

Closed

new CR: podnetworkconnectivitychecks.controlplane.operator.openshift.io/v1alpha1 #639

new CR: podnetworkconnectivitychecks.controlplane.operator.openshift.io/v1alpha1 #639

Conversation

sanchezl commented May 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts May 11, 2020 • edited

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

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

sttts commented May 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanchezl commented Jun 5, 2020

openshift-bot commented Jun 5, 2020

Choose a reason for hiding this comment

deads2k commented Jun 5, 2020

openshift-ci-robot commented Jun 5, 2020

sttts May 11, 2020 •

edited