Adding synchronization and other features to extended test cluster loader. #17894

jmencak · 2017-12-20T08:30:59Z

Fixes:

number of templates no longer ignored
tuningsets no longer ignored for templates
maxRetries reflected the number of all tries, not retries

New features:

label-based pod post deployment synchronization at pod, template
or global level
creation of config maps and secrets from files
support for getting parameter values from the environment

jmencak · 2017-12-20T08:32:07Z

@sjug creating this in parallel with #17072 for review purposes, while you're installing go ;-)

jmencak · 2018-01-02T10:28:01Z

Flakes on:

"ci/openshift-jenkins/cmd"
test/cmd/observe.sh:34: executing 'oc observe services --once --all-namespaces -a "bad{ .metadata.annotations.unset }key" --strict-templates' expecting failure and text 'annotations is not found': the output content test failed
ci/openshift-jenkins/extended_conformance_install
rm: cannot remove â€˜/tmp/etcd/memberâ€™: Permission denied

jmencak · 2018-01-02T13:13:08Z

Flakes on:

ci/openshift-jenkins/extended_conformance_gce
rm: cannot remove ‘/tmp/etcd’: Operation not permitted
ci/openshift-jenkins/integration
error retrieving resource lock kube-system/kube-controller-manager: Get https://127.0.0.1:21143/api/v1/namespaces/kube-system/configmaps/kube-controller-manager: dial tcp 127.0.0.1:21143: getsockopt: connection refused

jmencak · 2018-01-03T07:36:48Z

@sjug PTAL

sjug

A few changes, looking good overall.

sjug · 2018-01-03T13:42:50Z

test/extended/cluster/context.go

+	Number     int `mapstructure:"num"`
+	Basename   string
+	Tuning     string
+	Configmaps map[string]interface{}


nit, missing a space?

Not sure where, formatted by gofmt, also used go tool vet

The columns should align, not sure why you're not getting proper output.

Which columns do not align? I do not see any mis-alignment. Again, the code is formatted with "gofmt -s -d context.go" produces no diff. Please be specific.

The one I've tagged, line 26?
Configmaps is aligned, but map[string]interface{} is not.

It's just on github... ffs

Great, was getting desperate...

sjug · 2018-01-03T13:44:21Z

test/extended/cluster/context.go

@@ -33,9 +36,22 @@ type ClusterLoaderObjectType struct {
 	Image      string
 	Basename   string
 	File       string
+	Sync       SyncObjectType


Why is SyncObjectType nested in two structs?

In order to perform synchronization both on template/pod and cluster-loader wide level.

We need to discuss this further.

Not currently used but may be helpful in the future.

sjug · 2018-01-03T13:44:58Z

test/extended/cluster/context.go

+// SyncObjectType is nested object type for cluster loader synchronisation functionality
+type SyncObjectType struct {
+	Server struct {
+		Enable bool


nit, should probably be Enabled

sjug · 2018-01-03T13:53:47Z

test/extended/cluster/utils.go

-		if v != 0 && v != "" {
-			args = append(args, "-p")
-			args = append(args, fmt.Sprintf("%s=%v", k, v))
+		val := v


I think I understand what you're trying to do here, but it's not necessary as k & v are already new variables.

sjug · 2018-01-03T14:00:02Z

test/extended/cluster/utils.go

+			// Parameter not defined, see if it is defined in the environment.
+			var found bool
+			val, found = os.LookupEnv(fmt.Sprintf("%s", k))
+			if found == false {


found is boolean so we don't need to explicitly compare it.

if found { ...

sjug · 2018-01-03T14:12:15Z

test/extended/cluster/cl.go

+				// Create secrets
+				if p.Secrets != nil {
+					// Secrets defined
+					for k, v := range p.Secrets {


This block looks really similar to the configmap version L87-91 😄
Can we make it more generic too?

sjug · 2018-01-03T14:14:19Z

test/extended/cluster/utils.go

+	return args, err
+}
+
+func getSecretArgs(k string, v interface{}) (args []string, err error) {


nit, very similar to getConfigMapArgs.

sjug · 2018-01-03T14:32:07Z

test/extended/cluster/utils.go

@@ -41,12 +48,58 @@ func ParsePods(jsonFile string) (configStruct kapiv1.Pod) {
 	return
 }

+// Wait for pods to go into Running or "not Running" state
+func SyncPods(c kclientset.Interface, ns string, selectors map[string]string, timeout time.Duration, negate bool) (err error) {


This has already been implemented a bunch of times in the e2e framework (example). Do we want another version here to maintain?

So how do you implement waiting for a pod to go into anything but "Running" state using the code you pointed out?

Usually we would expect a specific state.

I do not see why. Expecting anything but "not Running" instead of "Complete/Error/..." is a valid reason for synchronization.

edit: On second thought, waiting for Complete + setting a timeout for complete might actually be a better idea. Will rewrite then.

sjug · 2018-01-03T14:33:39Z

test/extended/cluster/utils.go

 	}
+
+	return err


Shouldn't this be return nil or what error is this?

sjug · 2018-01-03T14:41:14Z

test/extended/cluster/cl.go

 				}
 			}
 		}

+		if sync.Running {
+			for _, ns := range namespaces {
+				err := SyncRunningPods(c, ns, sync.Selectors, time.Duration(sync.Timeout)*time.Second)


time.Duration(sync.Timeout)*time.Second

This kind of pattern has happened a lot in here, I suggest using time.ParseDuration rather than hardcoding. This is something I've done in the perf-tests cluster loader version.

jmencak · 2018-01-04T21:24:09Z

@sjug please check that all your concerns have been addressed.

sjug · 2018-01-05T02:30:32Z

test/extended/cluster/utils.go

-// Server is the webservice that will syncronize the start and stop of Pods
-func Server(c *PodCount) error {
+// Server is the webservice that will synchronize the start and stop of Pods
+func Server(c *PodCount, Port int, awaitShutdown bool) error {


Port should not be capitalized.

sjug · 2018-01-05T02:30:50Z

test/extended/cluster/utils.go

 	http.HandleFunc("/start", handleStart(startHandler, c))
 	http.HandleFunc("/stop", handleStop(stopHandler, c))
+	if Port <= 0 {


How could Port be < 0?

Perhaps because it is a signed int and user defined it as negative? Most code I've seen in origin uses int for ports.

I'd rather it just throw an error in that edge case, but this will be fine, maybe add a log message?

sjug · 2018-01-05T02:35:47Z

test/extended/cluster/utils.go

+	maxRetries = 4
+
+	// Poll every two seconds
+	Poll = 2 * time.Second


Not sure we need to export this?

…loader. Fixes: - number of templates no longer ignored - tuningsets no longer ignored for templates - maxRetries reflected the number of all tries, not retries New features: - label-based pod post deployment synchronization at pod, template or global level - creation of config maps and secrets from files - support for getting parameter values from the environment

jmencak · 2018-01-05T15:33:16Z

@sjug AFAIK all requests for change addressed.

sjug · 2018-01-05T17:39:52Z

/LGTM @jmencak

jmencak · 2018-01-06T20:32:45Z

/assign @stevekuznetsov

knobunc · 2018-01-08T21:14:48Z

/lgtm
@rajatchopra PTAL

knobunc · 2018-01-08T21:15:13Z

/approve

openshift-ci-robot · 2018-01-08T21:15:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmencak, knobunc, sjug

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~test/extended/OWNERS~~ [knobunc]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

jmencak · 2018-01-09T08:28:17Z

Flake on ci/openshift-jenkins/extended_conformance_gce "dial tcp 10.142.0.5:10250: getsockopt: connection refused"

jmencak · 2018-01-09T09:53:13Z

/test extended_conformance_gce

openshift-merge-robot · 2018-01-09T11:00:16Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-merge-robot · 2018-01-09T12:31:24Z

Automatic merge from submit-queue.

Documenting synchronization primitives and the possibility to create ConfigMaps and Secrets from files as implemented by openshift/origin#17894 (cherry picked from commit c85b8a9) xref:openshift#7297

jmencak requested a review from sjug December 20, 2017 08:30

openshift-ci-robot requested review from danwinship and knobunc December 20, 2017 08:31

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 20, 2017

danwinship removed their request for review December 20, 2017 14:20

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 21, 2017

jmencak force-pushed the cluster-loader-sync branch from ade2888 to 3adc981 Compare January 2, 2018 08:25

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 2, 2018

jmencak force-pushed the cluster-loader-sync branch 2 times, most recently from 29914ac to ca85550 Compare January 2, 2018 11:16

sjug reviewed Jan 3, 2018

View reviewed changes

jmencak force-pushed the cluster-loader-sync branch from ca85550 to 71f7984 Compare January 4, 2018 12:55

sjug suggested changes Jan 5, 2018

View reviewed changes

sjug reviewed Jan 5, 2018

View reviewed changes

jmencak force-pushed the cluster-loader-sync branch 5 times, most recently from de27297 to e4d5665 Compare January 5, 2018 12:21

jmencak force-pushed the cluster-loader-sync branch from e4d5665 to dd2366b Compare January 5, 2018 13:43

openshift-ci-robot assigned stevekuznetsov Jan 6, 2018

openshift-ci-robot assigned knobunc Jan 8, 2018

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 Jan 8, 2018

openshift-merge-robot merged commit 887de1b into openshift:master Jan 9, 2018

jmencak mentioned this pull request Jan 24, 2018

Documenting Cluster Loader's additional functionality. openshift/openshift-docs#7297

Merged

ahardin-rh mentioned this pull request Jan 24, 2018

[enterprise-3.9] Documenting Cluster Loader's additional functionality. openshift/openshift-docs#7310

Merged

jmencak deleted the cluster-loader-sync branch July 11, 2019 09:21

Adding synchronization and other features to extended test cluster loader. #17894

Adding synchronization and other features to extended test cluster loader. #17894

Conversation

jmencak commented Dec 20, 2017

jmencak commented Dec 20, 2017

jmencak commented Jan 2, 2018

jmencak commented Jan 2, 2018

jmencak commented Jan 3, 2018

sjug left a 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

jmencak Jan 5, 2018 • 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

sjug Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmencak Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmencak commented Jan 4, 2018

Choose a reason for hiding this comment

sjug Jan 5, 2018 • 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

jmencak commented Jan 5, 2018

sjug commented Jan 5, 2018

jmencak commented Jan 6, 2018

knobunc commented Jan 8, 2018

knobunc commented Jan 8, 2018

openshift-ci-robot commented Jan 8, 2018

jmencak commented Jan 9, 2018

jmencak commented Jan 9, 2018

openshift-merge-robot commented Jan 9, 2018

openshift-merge-robot commented Jan 9, 2018

jmencak Jan 5, 2018 •

edited

Loading

sjug Jan 3, 2018 •

edited

Loading

jmencak Jan 5, 2018 •

edited

Loading

sjug Jan 5, 2018 •

edited

Loading