Skip to content

Add job GeneratedName transformer to eventing, serving#1648

Merged
openshift-ci[bot] merged 5 commits into
openshift-knative:mainfrom
skonto:generate_storage_jobs_for_current_version
Jul 19, 2022
Merged

Add job GeneratedName transformer to eventing, serving#1648
openshift-ci[bot] merged 5 commits into
openshift-knative:mainfrom
skonto:generate_storage_jobs_for_current_version

Conversation

@skonto
Copy link
Copy Markdown
Collaborator

@skonto skonto commented Jul 18, 2022

Proposed Changes

  • Even if we have a different release suffix for the job name in the next few releases let's create unique names per S-O version.
  • Hit a related issue here: Bump to 1.25 #1647

@skonto skonto requested review from nak3 and pierDipi July 18, 2022 09:49
@openshift-ci openshift-ci Bot requested review from alanfx and jcrossley3 July 18, 2022 09:50
@skonto skonto mentioned this pull request Jul 18, 2022
@skonto skonto requested review from mgencur and removed request for alanfx and jcrossley3 July 18, 2022 09:50
@skonto skonto changed the title Add job transformer to eventing, serving Add job GeneratedName transformer to eventing, serving Jul 18, 2022
@mgencur
Copy link
Copy Markdown
Contributor

mgencur commented Jul 18, 2022

/lgtm
/hold
(for tests)

@skonto
Copy link
Copy Markdown
Collaborator Author

skonto commented Jul 18, 2022

Need to inject the CURRENT_VERSION. Tests fail without this.

@openshift-ci openshift-ci Bot removed the lgtm label Jul 18, 2022
@skonto
Copy link
Copy Markdown
Collaborator Author

skonto commented Jul 18, 2022

/retest

@pierDipi
Copy link
Copy Markdown
Member

@skonto
Copy link
Copy Markdown
Collaborator Author

skonto commented Jul 18, 2022

Yes I saw that let's see, not sure why no jobs is left there.

@pierDipi
Copy link
Copy Markdown
Member

Maybe the job's TTL is passed and the job removed when the test runs?

@skonto
Copy link
Copy Markdown
Collaborator Author

skonto commented Jul 18, 2022

I will probably move the checks at the beginning of the array as TTL is 600s and see if we have a better chance there.

@skonto
Copy link
Copy Markdown
Collaborator Author

skonto commented Jul 18, 2022

clusterrole.rbac.authorization.k8s.io/admin added: "user1"
Unable to connect to the server: dial tcp: lookup api.ci-ocp-4-10-amd64-aws-us-west-1-m6tc8.hive.aws.ci.openshift.org on 172.30.0.10:53: read udp 10.128.95.41:44754->172.30.0.10:53: i/o timeout
15:00:27.941 ERROR:   🚨 Error (code: 1) occurred at ./test/lib.bash:349, with command: oc adm policy add-role-to-user edit user2 -n "$TEST_NAMESPACE"
15:00:27.942 INFO:    Dumping state...

@skonto skonto changed the title Add job GeneratedName transformer to eventing, serving [wip] Add job GeneratedName transformer to eventing, serving Jul 18, 2022
@nak3
Copy link
Copy Markdown
Contributor

nak3 commented Jul 19, 2022

/retest

}

func verifyPostInstallServingJobs(ctx context.Context, testCtx *test.Context, c upgrade.Context, cfg VerifyPostJobsConfig) error {
jobs, err := testCtx.Clients.Kube.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function is completely same as verifyPostInstallJobs, except for initializing the clients. I think you could use the same approach as here for Eventing - get the clients from testCtx. Then this whole function could be reused and code duplication removed.

Copy link
Copy Markdown
Collaborator Author

@skonto skonto Jul 19, 2022

Choose a reason for hiding this comment

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

Eventing uses client := testlib.Setup(c.T, false) that does a lot more that is needed behind the scenes eg. try create a namespace. So It is not the same setup. I will remove the code duplication, I wanted first this to pass the tests. ;)
In general it seems Serving is fast but Eventing takes quite some time:

PASS test/upgrade.TestServerlessUpgrade/PostUpgradeTests/Verify_jobs_in_knative-serving (0.03s)
PASS test/upgrade.TestServerlessUpgrade/PostUpgradeTests/Verify_jobs_in_knative-eventing (607.77s)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And I think the testlib.Setup is not necessary because we only need the client to work with Jobs resources. We don't create any namespace here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah ok I cleaned it up for Eventing too ;)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

PASS test/upgrade.TestServerlessUpgrade/PostUpgradeTests/Verify_jobs_in_knative-serving (0.03s)
PASS test/upgrade.TestServerlessUpgrade/PostUpgradeTests/Verify_jobs_in_knative-eventing (607.77s)

That's huge 🤔

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think it is related to tear down logic. I hope unification to reduce time for Eventing too.

Copy link
Copy Markdown
Collaborator Author

@skonto skonto Jul 19, 2022

Choose a reason for hiding this comment

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

Still too slow:

PASS test/upgrade.TestServerlessUpgrade/PostUpgradeTests/Verify_jobs_in_knative-serving (5.07s)
PASS test/upgrade.TestServerlessUpgrade/PostUpgradeTests/Verify_jobs_in_knative-eventing (620.03s)

I guess polling the jobs takes quite some time there for Eventing. /cc @pierDipi

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, I know why, this will change when we bump eventing-kafka-broker (it is due to the KafkaChannel migration)

@skonto skonto changed the title [wip] Add job GeneratedName transformer to eventing, serving Add job GeneratedName transformer to eventing, serving Jul 19, 2022
@skonto
Copy link
Copy Markdown
Collaborator Author

skonto commented Jul 19, 2022

08:42:27.885 DEBUG:   Get the machineset with most replicas
The connection to the server api.ci-ocp-4-10-amd64-aws-us-west-1-zk4ff.hive.aws.ci.openshift.org:6443 was refused - did you specify the right host or port?
08:42:27.981 ERROR:   🚨 Error (code: 1) occurred at ./hack/lib/scaleup.bash:18, with command: current_total="$(oc get machineconfigpool worker -o 

@skonto
Copy link
Copy Markdown
Collaborator Author

skonto commented Jul 19, 2022

/retest

@skonto
Copy link
Copy Markdown
Collaborator Author

skonto commented Jul 19, 2022

@mgencur gentle ping.

@mgencur
Copy link
Copy Markdown
Contributor

mgencur commented Jul 19, 2022

/lgtm
/hold cancel

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jul 19, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mgencur, skonto

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:

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

@pierDipi
Copy link
Copy Markdown
Member


Unable to connect to the server: dial tcp: lookup api.ci-ocp-4-10-amd64-aws-us-east-1-rj6mp.hive.aws.ci.openshift.org on 172.30.0.10:53: read udp 10.130.198.212:49673->172.30.0.10:53: i/o timeout

/test 4.10-operator-e2e-aws-ocp-410

@openshift-ci openshift-ci Bot merged commit 3967754 into openshift-knative:main Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants