Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump code dependencies to 1.13 #2578

Conversation

ReToCode
Copy link
Member

@ReToCode ReToCode commented Mar 25, 2024

⚠️ See the separated commits for easier review (and because the GH UI dies if you open without filter 🤷)

Basic changes

  • Bump version of dependants to 1.12 (pointing to midstream 1.12 branches which are 1.13). So this is aligned with the yamls.
  • Bump operator to version 1.13 (as this is from upstream)
  • Re-order the dependencies to update upstream deps first. Otherwise the script was failing, as eventing deps depend on newer upstream deps
  • Removed K8s deps replacement, see conversation here and here
  • Updated the 006-version.patch to new controller-runtime version
  • Replaces the other approach

@openshift-ci openshift-ci bot requested review from aliok and matzew March 25, 2024 13:42

diff --git a/vendor/sigs.k8s.io/controller-runtime/pkg/client/config/config.go b/vendor/sigs.k8s.io/controller-runtime/pkg/client/config/config.go
index fd2772412..54aab6c3d 100644
--- a/vendor/sigs.k8s.io/controller-runtime/pkg/client/config/config.go
Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK this is no longer needed. But it might be good if someone can take another look.

@@ -29,13 +29,13 @@ FLOATING_DEPS=(
)

REPLACE_DEPS=(
"knative.dev/eventing-kafka-broker=github.com/openshift-knative/eventing-kafka-broker@${EVENTING_KAFKA_BROKER_VERSION}"
Copy link
Member Author

Choose a reason for hiding this comment

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

reordered to make sure the script runs. Eventing deps need new version of upstream deps first

@@ -66,13 +66,25 @@ func main() {
os.Exit(1)
}

// Setup all Webhooks
disableHTTP2 := func(c *tls.Config) { c.NextProtos = []string{"http/1.1"} }
Copy link
Member Author

Choose a reason for hiding this comment

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

this is not longer exposed, to have a "custom" webhook, one needs to create it upfront and pass it to the manager. IMHO, might be good if somebody else can cross-check.

@ReToCode
Copy link
Member Author

ReToCode commented Mar 25, 2024

/override "Lint"

Let's fix this in a separate PR.

Copy link
Contributor

openshift-ci bot commented Mar 25, 2024

@ReToCode: Overrode contexts on behalf of ReToCode: Lint

In response to this:

/override "Lint"

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-knative openshift-knative deleted a comment from openshift-ci bot Mar 25, 2024
@pierDipi pierDipi self-requested a review March 25, 2024 15:35
@ReToCode
Copy link
Member Author

/test 415-test-upgrade-aws-415

@ReToCode
Copy link
Member Author

/test 415-upstream-e2e-kafka-aws-415

@pierDipi
Copy link
Member

  * could not run steps: step upstream-e2e-kafka-aws-415 failed: "upstream-e2e-kafka-aws-415" test steps failed: "upstream-e2e-kafka-aws-415" pod "upstream-e2e-kafka-aws-415-test" failed: could not watch pod: the pod ci-op-tnbv63vn/upstream-e2e-kafka-aws-415-test failed after 0s (failed containers: ): Evicted Pod was rejected: The node had condition: [DiskPressure].

@pierDipi
Copy link
Member

pierDipi commented Mar 26, 2024

On the previous run, for TestRestrictedBrokerAuthSslSaslScram512

panic: secrets "my-restricted-sasl-user" not found [recovered]
	panic: secrets "my-restricted-sasl-user" not found
goroutine 5002 [running]:
testing.tRunner.func1.2({0x31b1ba0, 0xc00845e000})
	/usr/lib/golang/src/testing/testing.go:1545 +0x3f7
testing.tRunner.func1()
	/usr/lib/golang/src/testing/testing.go:1548 +0x716
panic({0x31b1ba0?, 0xc00845e000?})
	/usr/lib/golang/src/runtime/panic.go:920 +0x270
knative.dev/eventing-kafka-broker/test/rekt/resources/kafkaauthsecret.WithRestrictedSslSaslScram512Data({0x3a39ed8, 0xc00d0c7320})
	/go/src/knative.dev/eventing-kafka-broker/test/rekt/resources/kafkaauthsecret/kafkaauthsecret.go:115 +0x63b
knative.dev/eventing-kafka-broker/test/rekt/features.SetupBrokerAuthRestrictedSslSaslScram512({0x3a39ed8, 0xc00d0c7320})
	/go/src/knative.dev/eventing-kafka-broker/test/rekt/features/broker_auth.go:69 +0x35
knative.dev/eventing-kafka-broker/test/e2e_new.TestRestrictedBrokerAuthSslSaslScram512(0xc0067a3ba0)
	/go/src/knative.dev/eventing-kafka-broker/test/e2e_new/broker_sasl_ssl_test.go:113 +0x47b
testing.tRunner(0xc0067a3ba0, 0x3597878)
	/usr/lib/golang/src/testing/testing.go:1595 +0x262
created by testing.(*T).Run in goroutine 1
	/usr/lib/golang/src/testing/testing.go:1648 +0x846

@pierDipi
Copy link
Member

pierDipi commented Mar 26, 2024

@ReToCode for the Kafka test, we need to apply the new KafkaUser here openshift-knative/eventing-kafka-broker@ef01093

In SO we apply KafkaUsers here

function install_strimzi_users {

@pierDipi
Copy link
Member

I can do it as well, if you don't want to

@ReToCode
Copy link
Member Author

Also something is off with the registry:

podman pull registry.ci.openshift.org/knative/serverless-knative-operator:release-1.32
Trying to pull registry.ci.openshift.org/knative/serverless-knative-operator:release-1.32...
Error: initializing source docker://registry.ci.openshift.org/knative/serverless-knative-operator:release-1.32: reading manifest release-1.32 in registry.ci.openshift.org/knative/serverless-knative-operator: manifest unknown

@ReToCode
Copy link
Member Author

I can do it as well, if you don't want to

feel free to commit to this branch, might be easier as you exactly know what to do :)

@pierDipi
Copy link
Member

Done! Let's see ...

@ReToCode
Copy link
Member Author

Done! Let's see ...

Looks good, thanks @pierDipi. Now we need the fixes for the 1.32 images and hopefully the upgrade tests also pass.

@ReToCode
Copy link
Member Author

/test 415-test-upgrade-aws-415

@ReToCode
Copy link
Member Author

/override "Lint"

Copy link
Contributor

openshift-ci bot commented Mar 26, 2024

@ReToCode: Overrode contexts on behalf of ReToCode: Lint

In response to this:

/override "Lint"

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.

@maschmid
Copy link
Contributor

maschmid commented Mar 26, 2024

the upgrade test DeploymentFailurePostUpgrade failure is probably because our current version does not have the knative/serving#14795 fix ... upstream had the same issue when they introduced the upgrade test with the fix knative/serving#14795 (comment)

@skonto
Copy link
Contributor

skonto commented Mar 26, 2024

/hold

@ReToCode
Copy link
Member Author

The latest rebase https://github.com/openshift-knative/serverless-operator/compare/c20051d59d1a1fd5176591eaa4e526bf8652002a..220c6be9141e112b884cdc488c818893006480a7 seems to have dropped the addition of the my-restricted-sasl-user kafkauser that was added in c20051d ?

No idea what I did wrong there 🤷 But nice catch.

@ReToCode
Copy link
Member Author

/override "ci/prow/415-test-upgrade-aws-415"

/test 412-operator-e2e-aws-412

/test 412-test-upgrade-aws-412

/test 412-upstream-e2e-kafka-aws-412

/test 412-upstream-e2e-aws-412

@openshift-knative openshift-knative deleted a comment from openshift-ci bot Mar 28, 2024
@openshift-knative openshift-knative deleted a comment from openshift-ci bot Mar 28, 2024
@openshift-knative openshift-knative deleted a comment from openshift-ci bot Mar 28, 2024
@ReToCode
Copy link
Member Author

ReToCode commented Mar 28, 2024

/override "ci/prow/415-operator-e2e-aws-415"

/override "ci/prow/415-test-upgrade-aws-415"

/override "ci/prow/415-upstream-e2e-kafka-aws-415"

/override "ci/prow/415-upstream-e2e-aws-415"

Copy link
Contributor

openshift-ci bot commented Mar 28, 2024

@ReToCode: Overrode contexts on behalf of ReToCode: ci/prow/415-operator-e2e-aws-415, ci/prow/415-test-upgrade-aws-415

In response to this:

/override "ci/prow/415-operator-e2e-aws-415"

/override "ci/prow/415-test-upgrade-aws-415"

/test "ci/prow/415-upstream-e2e-kafka-aws-415"

/test "ci/prow/415-upstream-e2e-aws-415"

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-knative openshift-knative deleted a comment from openshift-ci bot Mar 28, 2024
@openshift-knative openshift-knative deleted a comment from openshift-ci bot Mar 28, 2024
@ReToCode
Copy link
Member Author

/test 412-operator-e2e-aws-412

@ReToCode
Copy link
Member Author

    prober.go:106: event #15855 should be received once, but was received 0 times
    prober.go:106: expecting to have 15860 unique events received, but received 15452 unique events
    logger.go:146: 2024-03-28T13:47:34.908Z	DEBUG	Finished "BrokerBackedByChannelContinualTest"

/test 412-test-upgrade-aws-412

@mgencur
Copy link
Contributor

mgencur commented Mar 28, 2024

For the BrokerBackedByChannelContinualTest test failing, it would be good to check if the events are dropped during upgrade OR downgrade (based on timestamps). The eventing continual tests showed real issues with the previous release during downgrade. There might be a new problem now.

@ReToCode
Copy link
Member Author

For the BrokerBackedByChannelContinualTest test failing, it would be good to check if the events are dropped during upgrade OR downgrade (based on timestamps). The eventing continual tests showed real issues with the previous release during downgrade. There might be a new problem now.

I don't really know about that - or how to do that. I just wanted to retry because it passed before.

@pierDipi
Copy link
Member

pierDipi commented Mar 28, 2024

I'm looking at #13422 (reported as missing) [1] and on the trace https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift-knative_serverless-operator/2578/pull-ci-openshift-knative-serverless-operator-main-412-test-upgrade-aws-412/1773327266730741761/artifacts/test-upgrade-aws-412/test/artifacts/build-Wr07zAdI/traces/events/step-13422.json I see that the filter received a 200 from the wathola-receiver

  {
    "timestamp": 1711633439448110,
    "duration": 964,
    "traceId": "48de43c827b9a107e5b6fe9a0e32d06b",
    "id": "7acadf20123aa4fc",
    "parentId": "6b40d5b36d3ef866",
    "kind": "CLIENT",
    "localEndpoint": {
      "serviceName": "broker-filter.knative-eventing",
      "ipv4": "172.30.195.148"
    },
    "tags": {
      "http.host": "wathola-receiver.eventing-e2e1.svc.cluster.local",
      "http.method": "POST",
      "http.path": "",
      "http.status_code": "200",
      "http.url": "http://wathola-receiver.eventing-e2e1.svc.cluster.local",
      "opencensus.status_description": "OK"
    }
  },

[1] prober.go:106: event #13422 should be received once, but was received 0 times

@pierDipi
Copy link
Member

We're "missing" these 2 improvements on 1.13

@pierDipi
Copy link
Member

Porting them here openshift-knative/eventing#580, I think "Wait for events with poll interval after finished event received" might help

@ReToCode
Copy link
Member Author

ReToCode commented Apr 2, 2024

@pierDipi can you restart the test once the fixes are in?

@pierDipi
Copy link
Member

pierDipi commented Apr 2, 2024

/test 415-test-upgrade-aws-415

@pierDipi
Copy link
Member

pierDipi commented Apr 2, 2024

/test 412-test-upgrade-aws-412

@ReToCode
Copy link
Member Author

ReToCode commented Apr 3, 2024

Cool, 4.12 builds now all pass 🎉 Seems like the 4.15 ones still fail cause of the cluster-operators bug. Please take another look at the PR @pierDipi @maschmid @skonto @mgencur.

/override "Lint"

/override "ci/prow/415-operator-e2e-aws-415"

/override "ci/prow/415-test-upgrade-aws-415"

/override "ci/prow/415-upstream-e2e-kafka-aws-415"

/override "ci/prow/415-upstream-e2e-aws-415"

Copy link
Contributor

openshift-ci bot commented Apr 3, 2024

@ReToCode: Overrode contexts on behalf of ReToCode: Lint, ci/prow/415-operator-e2e-aws-415, ci/prow/415-test-upgrade-aws-415, ci/prow/415-upstream-e2e-aws-415, ci/prow/415-upstream-e2e-kafka-aws-415

In response to this:

Cool, 4.12 builds now all pass 🎉 Seems like the 4.15 ones still fail cause of the cluster-operators bug. Please take another look at the PR @pierDipi @maschmid @skonto @mgencur.

/override "Lint"

/override "ci/prow/415-operator-e2e-aws-415"

/override "ci/prow/415-test-upgrade-aws-415"

/override "ci/prow/415-upstream-e2e-kafka-aws-415"

/override "ci/prow/415-upstream-e2e-aws-415"

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.

Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Copy link
Contributor

openshift-ci bot commented Apr 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierDipi, ReToCode

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:

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
Member

pierDipi commented Apr 3, 2024

Thanks @ReToCode

@pierDipi
Copy link
Member

pierDipi commented Apr 3, 2024

If you're happy with this PR, I think we need to manually merge as lint errors cannot be made green nor fixed

@openshift-merge-bot openshift-merge-bot bot merged commit 355521c into openshift-knative:main Apr 3, 2024
26 of 27 checks passed
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.

None yet

6 participants