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.

Sign up for GitHub

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

Jump to bottom

Bug 1985997: wires the startup monitor #1177

Merged
merged 4 commits into from Jul 26, 2021
Merged

Bug 1985997: wires the startup monitor #1177

merged 4 commits into from Jul 26, 2021

Conversation

p0lyn0mial
Copy link
Contributor

@p0lyn0mial p0lyn0mial commented Jul 15, 2021 •
edited

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2021
@openshift-ci openshift-ci bot requested review from soltysh and sttts July 15, 2021 09:04
@p0lyn0mial
Copy link
Contributor Author

/assign @sttts

p0lyn0mial
p0lyn0mial commented Jul 15, 2021
View reviewed changes
pkg/operator/startupmonitorreadiness/readyz_check.go Outdated
var _ startupmonitor.HealthChecker = &KubeAPIReadyzChecker{}
var _ startupmonitor.WantsRestConfig = &KubeAPIReadyzChecker{}

func New() *KubeAPIReadyzChecker {
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 will be a chain of ordered checks to apply

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2021
@p0lyn0mial p0lyn0mial changed the title [WIP] wires the startup monitor wires the startup monitor Jul 23, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 23, 2021
p0lyn0mial
p0lyn0mial commented Jul 23, 2021
View reviewed changes
pkg/operator/targetconfigcontroller/targetconfigcontroller.go
// we got an error so without the infrastructure object we are not able to determine the type of platform we are running on
return "", nil, err
}
if infra.Status.ControlPlaneTopology != configv1.SingleReplicaTopologyMode {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sttts you wanted to have an unsupported flag for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

at least in addition. I want to test in a cluster-bot HA cluster.

sttts
sttts reviewed Jul 23, 2021
View reviewed changes
pkg/operator/targetconfigcontroller/targetconfigcontroller.go Outdated
return resourceapply.ApplyConfigMap(ctx, client, recorder, configMap)
}

func manageOptionalStartupMonitorPod(infrastructureLister configlistersv1.InfrastructureLister, operatorSpec *operatorv1.StaticPodOperatorSpec, operatorImagePullSpec string) (string, *corev1.Pod, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this side-effect free or actively creating a pod? Only call it "manage*" in the later case. Otherwise, get or create.

sttts
sttts reviewed Jul 23, 2021
View reviewed changes
pkg/operator/targetconfigcontroller/targetconfigcontroller.go Outdated
return "", nil, nil
}

appliedStartupMonitorPodTemplate, err := startupmonitor.ManagePodTemplate(operatorSpec, []string{"cluster-kube-apiserver-operator", "startup-monitor"}, operatorclient.TargetNamespace, "kube-apiserver", operatorImagePullSpec)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here: ManagePodTemplate does not do any API action, does it? Better call it (in library-go) generate.

@sttts
Copy link
Contributor

sttts commented Jul 23, 2021

/lgtm
/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 Jul 23, 2021
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

11 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci openshift-ci bot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Jul 26, 2021
@p0lyn0mial
Copy link
Contributor Author

/bugzilla refresh

@openshift-ci openshift-ci bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. labels Jul 26, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 26, 2021

@p0lyn0mial: This pull request references Bugzilla bug 1985997, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.9.0) matches configured target release for branch (4.9.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @wangke19

In response to this:

/bugzilla refresh

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 openshift-ci bot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Jul 26, 2021
@openshift-ci openshift-ci bot requested a review from wangke19 July 26, 2021 13:15
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci openshift-ci bot mentioned this pull request Jul 26, 2021
Merged
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 26, 2021 •
edited

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

Test name Commit Details Rerun command
ci/prow/e2e-aws-single-node 5989154 link /test e2e-aws-single-node

Full PR test history. Your PR dashboard.

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.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit d5f73af into openshift:master Jul 26, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 26, 2021

@p0lyn0mial: All pull requests linked via external trackers have merged:

Bugzilla bug 1985997 has been moved to the MODIFIED state.

In response to this:

Bug 1985997: wires the startup monitor

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.

mfojtik pushed a commit to mfojtik/cluster-kube-apiserver-operator that referenced this pull request Jul 29, 2021
@openshift-merge-robot @mfojtik
Merge pull request openshift#1177 from p0lyn0mial/wire-startup-monitor
2c1f59e 
Bug 1985997: wires the startup monitor
This was referenced Aug 2, 2021
Merged
Merged
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@soltysh soltysh Awaiting requested review from soltysh

@wangke19 wangke19 Awaiting requested review from wangke19

Assignees

@sttts sttts

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@p0lyn0mial @sttts @openshift-bot @openshift-merge-robot