Skip to content

Conversation

@pacevedom
Copy link
Contributor

No description provided.


var (
microShiftVersion string
microShiftMutex sync.Mutex
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this protecting against?

@pacevedom
Copy link
Contributor Author

/hold
Need to get rid of microshift checks.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 23, 2022
@pacevedom
Copy link
Contributor Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2022
})

g.Describe("Unidling", func() {
g.Describe("Unidling [apigroup:apps.openshift.io]", func() {
Copy link
Member

Choose a reason for hiding this comment

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

route.openshift.io as well (both apps and route are used in testdata/idling-echo-server.yaml (set through echoServerFixture)

Copy link
Member

Choose a reason for hiding this comment

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

Also with a single service and DeploymentConfig (line 218) is using route.openshift.io (through testdata/idling-echo-server-rc.yaml)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}, metav1.CreateOptions{})
}

_, err = oc.AdminKubeClient().RbacV1().RoleBindings(ns).Create(context.Background(), roleBinding, metav1.CreateOptions{})
Copy link
Member

Choose a reason for hiding this comment

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

Why this change when roleBinding is used only once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leftover from previous change.

}

func FindRouterImage(oc *CLI) (string, error) {
microShift, err := IsMicroShiftDeployment(oc)
Copy link
Member

Choose a reason for hiding this comment

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

Leftover? IsMicroShiftDeployment is never defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return "", err
}
if !exists {
deployment, err := oc.AdminKubeClient().AppsV1().Deployments("openshift-ingress").Get(context.Background(), "router-default", metav1.GetOptions{})
Copy link
Member

Choose a reason for hiding this comment

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

In 4.12 taking deployment.Spec.Template.Spec.Containers[0].Image from the deployment gets me quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:a955bf1686fcb4f11cd23880be6025c2dc75252d318931211554be86f4f5d0f1. Taking .Status.Versions[*].Version from clusteroperator/ingress I get quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:a955bf1686fcb4f11cd23880be6025c2dc75252d318931211554be86f4f5d0f1. @pacevedom is it always guaranteed the images will be identical?

Copy link
Member

@ingvagabund ingvagabund Oct 4, 2022

Choose a reason for hiding this comment

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

@openshift/networking can you confirm accessing deployment.Spec.Template.Spec.Containers[0].Image of router-default deployment gets the same image as from the ingress clusteroperator object? Resp. is this acceptable alternative?

Copy link
Contributor

@Miciah Miciah Oct 4, 2022

Choose a reason for hiding this comment

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

Yes, deployment.Spec.Template.Spec.Containers[0].Image of the openshift-ingress/router-default deployment on both MicroShift and standard OpenShift is the same image as the "ingress-controller" image in the ingress clusteroperator on standard OpenShift. This approach looks reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is output from a 4.12 cluster:

$ oc get deployment -n openshift-ingress router-default -o jsonpath='{.spec.template.spec.containers[*].image}'
quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:72c751aa148bf498839e6f37b304e3265f85af1e00578e637332a13ed9545ece

$ oc get clusteroperator ingress -o jsonpath='{.status.versions[?(@.name=="ingress-controller")].version}'
quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:72c751aa148bf498839e6f37b304e3265f85af1e00578e637332a13ed9545ece

router-default is MicroShift equivalent for ingress-controller pod in OCP.

@ingvagabund
Copy link
Member

/lgtm
based on #27428 (comment)

Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci openshift-ci bot 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 Oct 4, 2022
@pacevedom
Copy link
Contributor Author

/retest-required

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 9edb6c7 and 2 for PR HEAD 0feb162 in total

@pacevedom
Copy link
Contributor Author

/retest-required

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD efd2179 and 1 for PR HEAD 0feb162 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 703e3d1 and 0 for PR HEAD 0feb162 in total

@openshift-ci-robot
Copy link

/hold

Revision 0feb162 was retested 3 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 5, 2022
@pacevedom
Copy link
Contributor Author

/unhold
/retest-required

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 5, 2022
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 703e3d1 and 2 for PR HEAD 0feb162 in total

@pacevedom
Copy link
Contributor Author

/retest-required

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 6f17d51 and 2 for PR HEAD 0feb162 in total

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2022
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2022
{
name: "basic",
testLine: "ok package/name 0.160s",
name: "basic",
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is formatted with go1.18. @pacevedom the codebase has been recently formatted with go1.19.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2022
@ingvagabund
Copy link
Member

/lgtm

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2022
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2022
@ingvagabund
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 26, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund, pacevedom, soltysh

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

@pmtk
Copy link
Member

pmtk commented Oct 26, 2022

/retest-required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 26, 2022

@pacevedom: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-agnostic-ovn-cmd 8d9355e link false /test e2e-agnostic-ovn-cmd
ci/prow/e2e-gcp-ovn-rt-upgrade 8d9355e link false /test e2e-gcp-ovn-rt-upgrade
ci/prow/e2e-aws-ovn-serial 8d9355e link true /test e2e-aws-ovn-serial
ci/prow/e2e-aws-ovn-single-node 8d9355e link false /test e2e-aws-ovn-single-node
ci/prow/e2e-aws-ovn-single-node-serial 8d9355e link false /test e2e-aws-ovn-single-node-serial
ci/prow/e2e-gcp-csi 8d9355e link false /test e2e-gcp-csi
ci/prow/e2e-aws-csi 8d9355e link false /test e2e-aws-csi
ci/prow/e2e-metal-ipi-ovn-ipv6 8d9355e link false /test e2e-metal-ipi-ovn-ipv6
ci/prow/e2e-aws-ovn-fips 8d9355e link true /test e2e-aws-ovn-fips
ci/prow/e2e-openstack-ovn 8d9355e link false /test e2e-openstack-ovn
ci/prow/e2e-metal-ipi-sdn 8d9355e link false /test e2e-metal-ipi-sdn
ci/prow/e2e-aws-ovn-cgroupsv2 8d9355e link false /test e2e-aws-ovn-cgroupsv2
ci/prow/e2e-gcp-ovn 8d9355e link true /test e2e-gcp-ovn
ci/prow/e2e-aws-ovn-single-node-upgrade 8d9355e link false /test e2e-aws-ovn-single-node-upgrade

Full PR test history. Your PR dashboard.

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.

@ingvagabund
Copy link
Member

ingvagabund commented Oct 26, 2022

From https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/27428/pull-ci-openshift-origin-master-e2e-aws-ovn-fips/1585305880637739008:

[sig-network-edge][Feature:Idling] Unidling [apigroup:apps.openshift.io][apigroup:route.openshift.io] should handle many TCP connections by possibly dropping those over a certain bound [Serial] [Suite:openshift/conformance/serial] is constantly failing. Because it's originally skipped by the name changed: https://github.com/openshift/origin/blob/master/test/extended/util/annotate/rules.go#L68 and https://github.com/openshift/origin/blob/master/test/extended/util/annotate/rules.go#L307-L308

@ingvagabund
Copy link
Member

Merged through #27498

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2022
@openshift-merge-robot
Copy link
Contributor

@pacevedom: 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.

@ingvagabund
Copy link
Member

/close

@openshift-ci openshift-ci bot closed this Oct 27, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 27, 2022

@ingvagabund: 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants