prowgen: IaaS-agnostic src installer tests #21

bbguimaraes · 2019-07-10T09:15:10Z

No description provided.

This commit is a temporary solution and should be reverted when the template gets created. It is not worth it to create a separate ConfigMap for this type of test. Tests will fail because of the missing template, but they wouldn't otherwise be able to be scheduled due to the missing template. These tests are supposed to be non-blocking and UPI tests are currently configured to run 2% of the time, so the impact should be minimal.

openshift-ci-robot · 2019-07-10T09:15:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bbguimaraes

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [bbguimaraes]

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

petr-muller

Few initial observations, haven't looked at everything.

petr-muller · 2019-07-10T10:06:57Z

test/prowgen-integration/data/input/config/super/duper/super-duper-master.yaml

  openshift_installer_random: {}
+- as: e2e-random-src
+  commands: make e2e
+  openshift_installer_src_random: {}


I know this only follows what was done for the first template, but I only noticed now. If we intend ci-operator files to be usable (readable/writable) by humans, we should avoid non-intuitive clutter like this, even for a cost of slightly more complicated implementation. How hard would it be to make it:

- as: ... openshift_installer_src: cluster_profile: random

?

It's probaby not a matter of this PR, but I think we're still early enough in the IaaS-agnostic job work to make that change.

That makes sense, I didn't really think about it too long before deciding on the name. How about omitting the cluster profile altogether? I don't really like random and I think "not tied to a specific target" is a good default. This way we can remove random from the job type as well.

I liked what you proposed at first (even wrote a comment in that direction), but after more thinking I think we should keep the explicit config. I think the agnostic behavior could make a good default, but its not intuitive to whoever is trying to read and write these configs. We know every bit and quirk of it, but that's not the case for everyone.

(to be entirely clear, I did not originally like the {} part of the config)

Some API Objects have a type field for this purpose when the type-specific config does not exist or is optional

Ignoring the problem of how to structure the configuration (more on that below) I think random being the default value makes the configuration more intuitive, not less.

If the test needs a cluster, use openshift_installer/openshift_installer_src. If it needs a cluster with a specific configuration, add cluster_profile: …. The former should be the default choice for test authors if the test templates do their job and all supported targets are virtually equivalent. The details of what cluster is created should not concern them.

As for the configuration, we have these test types (ignoring ansible, console, and the two random types I am proposing we remove):

container

openshift_installer

openshift_installer_src

openshift_installer_upi

container tests must have a from field. openshift_installer* tests must have a cluster_type.

I don't particularly like the empty dictionary, but there are examples even in Kubernetes (e.g.). Adding an extra field to all types seems silly in this case: unless we default the value if one of the pointers is set, which doesn't seem very ellegant either, we would end up with:

type: container container: # …

Additionally, if we ever add a field to the cluster types (openshift_installer has an optional upgrade field), that becomes entirely redundant.

I think random being the default value makes the configuration more intuitive, not less.

I'm trying to imagine this from the PoV of a generic OpenShift engineer who does not know ci-operator configs very well. Currently the config is telling them quite well what they have set up, "smart" defaults defeat that.

The details of what cluster is created should not concern them: This IMHO makes sense only from high-level, not from individual's PoV.

Not really, the point of k8s and openshift in general is to be portable cross-cloud and not worry at all, so the default idea that it matters not which infra is underneath is a good one.

Good discussion. I'm not entirely convinced, but I do not need to be. I'm OK with making it that way.

petr-muller · 2019-07-10T10:08:48Z

pkg/api/types.go

 // chosen randomly and runs conformance tests.
 type OpenshiftInstallerRandomClusterTestConfiguration struct{}

+// OpenshiftInstallerSrcRandomClusterTestConfiguration describes a


// ... describes a <SOMETHING MISSING> // that provisions ...

Oh, someone actually reads those? =p

petr-muller · 2019-07-10T11:24:48Z

cmd/ci-operator-prowgen/main.go

-			if conf := element.OpenshiftInstallerRandomClusterTestConfiguration; conf != nil {
+			if element.OpenshiftInstallerRandomClusterTestConfiguration != nil {
+				podSpec = generatePodSpecRandom(info, &element)
+			} else if element.OpenshiftInstallerSrcRandomClusterTestConfiguration != nil {


Both branches are the same, is this intentional? Looks correct, but then an OR instead of a separate if may be in order.

It was supposed to keep the line short(er), using those names is a bit ridiculous. I'll change to an ||.

stevekuznetsov

Agreed that empty config looks weird

stevekuznetsov · 2019-07-10T16:41:33Z

test/prowgen-integration/data/input/config/super/duper/super-duper-master.yaml

  openshift_installer_random: {}
+- as: e2e-random-src
+  commands: make e2e
+  openshift_installer_src_random: {}


Some API Objects have a type field for this purpose when the type-specific config does not exist or is optional

openshift-ci-robot · 2019-07-19T21:32:20Z

@bbguimaraes: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2019-08-12T05:47:39Z

@bbguimaraes: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/validate-vendor	`c96d357`	link	`/test validate-vendor`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

stevekuznetsov · 2019-09-03T17:34:58Z

/close

openshift-ci-robot · 2019-09-03T17:34:59Z

@stevekuznetsov: Closed this PR.

Details

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

# This is the 1st commit message: force all disruption aggregation to skipped status while we re-gather unbugged data # This is the commit message openshift#2: Fix lint issue. # This is the commit message openshift#3: sanitize: keep jobs from the same PR on the same cluster # This is the commit message openshift#4: Support runIfChanged and skipIfOnlyChanged for postsubmit Signed-off-by: clyang82 <chuyang@redhat.com> # This is the commit message openshift#5: Update UT Signed-off-by: clyang82 <chuyang@redhat.com> # This is the commit message openshift#6: fix integration test Signed-off-by: clyang82 <chuyang@redhat.com> # This is the commit message openshift#7: [DPTP-2635]cluster-display: add endpont for clusters # This is the commit message openshift#8: prowgen: stop tolerating changes to `optional` fields ...except for images jobs for which do not have syntax in ci-operator config Once we pull all manual changes to ci-operator configs, we can stop tolerating these changes. One step closer to fully generated Prowjob configuration. # This is the commit message openshift#9: Honor semver when comparing clone target/source # This is the commit message openshift#10: Write a README for autoconfigbrancher # This is the commit message openshift#11: autoconfigbrancher: simplify a conditional # This is the commit message openshift#12: autoconfigbrancher: use a permanent link in README # This is the commit message openshift#13: Rename mis-spelled variable, add short for cobra help, add comments # This is the commit message openshift#14: add more comments re: GCS # This is the commit message openshift#15: Note two tables created in memory using a SELECT # This is the commit message openshift#16: fix typo # This is the commit message openshift#17: cast to a type so we can see more info on dryrun mode # This is the commit message openshift#18: Share ReleaseController's config # This is the commit message openshift#19: Add aggregating job results to testgrid (openshift#2576) * Add aggregating job results to testgrid * Add aggregating job results to testgrid Aggregating jobs are added to blocking dashboard of the corresponding release. Release is determined by the aggregated job prow config. * Add test cases * Add nil pointer check # This is the commit message openshift#20: Updating auto-include logic for OpenStack jobs Adding the OpenStack jobs which recently moved from release to shiftstack directory. This will add them to testgrid automatically. # This is the commit message openshift#21: [DPTP-2660]payload-testing-prow-plugin: format errors # This is the commit message openshift#22: handle missing history for aggregated disruption # This is the commit message openshift#23: Add create-tables subcommand to create Jobs tab and debugging tips to README # This is the commit message openshift#24: create JobRuns table # This is the commit message openshift#25: Rename vars to be more generic for table creation # This is the commit message openshift#26: add in gofmt fixes for lint # This is the commit message openshift#27: comment about Jobs being initially primed # This is the commit message openshift#28: comment compile time checks in case go beginners have never seen it # This is the commit message openshift#29: prpqr: add a PR deploy makefile target # This is the commit message openshift#30: prpqr ui: use centos stream instead of 8 from docker # This is the commit message openshift#31: Refuse /payload command on PRs to repositories that do not contribute to OCP # This is the commit message openshift#32: allow correcting history without changing test name when we get criteria incorrect # This is the commit message openshift#33: switch the check for aggregation passes to use the SQL views

bbguimaraes added 2 commits July 10, 2019 09:09

prowgen: IaaS-agnostic src installer tests

4326a5b

openshift-ci-robot requested review from droslean and petr-muller July 10, 2019 09:15

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 10, 2019

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

bbguimaraes mentioned this pull request Jul 10, 2019

openshift/console: add IaaS-agnostic E2E test openshift/release#4325

Closed

petr-muller reviewed Jul 10, 2019

View reviewed changes

stevekuznetsov reviewed Jul 10, 2019

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2019

openshift-ci-robot closed this Sep 3, 2019

bbguimaraes deleted the iaas_src branch September 18, 2019 19:00

prowgen: IaaS-agnostic src installer tests #21

prowgen: IaaS-agnostic src installer tests #21

Uh oh!

Conversation

bbguimaraes commented Jul 10, 2019

Uh oh!

openshift-ci-robot commented Jul 10, 2019

Uh oh!

petr-muller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petr-muller Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevekuznetsov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jul 19, 2019

Uh oh!

openshift-ci-robot commented Aug 12, 2019

Uh oh!

stevekuznetsov commented Sep 3, 2019

Uh oh!

openshift-ci-robot commented Sep 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

petr-muller Jul 12, 2019 •

edited

Loading