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

Jump to bottom

monitoring remove chattiness and race condition waits #1405

Merged
merged 2 commits into from Apr 4, 2019
Merged

monitoring remove chattiness and race condition waits #1405

merged 2 commits into from Apr 4, 2019

Conversation

mjudeikis
Copy link
Contributor

@mjudeikis mjudeikis commented Apr 2, 2019 •
edited 

NONE

fix: #1398
fix: #1399
remove chattiness and adds waits for race conditions

@openshift-ci-robot openshift-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 2, 2019
@mjudeikis mjudeikis changed the title monitoring remove chattiness monitoring remove chattiness and race condition waits Apr 2, 2019
@mjudeikis
Copy link
Contributor Author

/retest
no containers - no logs... hmmm

jim-minter
jim-minter reviewed Apr 2, 2019
View reviewed changes
cmd/monitoring/monitoring.go Outdated
hostnames = []string{}
for iter, err := m.pipcli.ListVirtualMachineScaleSetPublicIPAddressesComplete(ctx, resourceGroup, "ss-master"); iter.NotDone(); err = iter.Next() {
if err != nil {
return nil, err
time.Sleep(5 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we not lose IP addresses if we do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might. But we will see less IP in the metrics.
If this returns err - we have a bigger problem. Not sure we want to try catching ARM errors here as it should be part of plugin code

Copy link
Contributor

Choose a reason for hiding this comment

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

we're not aligned. these days the fake rp writes _data/containerservice.yaml before (and after) the cluster is created so you will end up here before the IPs exist. If you can, poll locally to be sure the cluster is created (can you read the provisioning state perhaps?) then fetch all the IPs. Adding time.Sleep(5 * time.Second) without a loop basically achieves nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is issue with it now: #1416 Fixing in separate PR.

@mjudeikis
Copy link
Contributor Author

/retest

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 2, 2019
@mjudeikis
Copy link
Contributor Author

/retest

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 3, 2019
@codecov
Copy link

codecov bot commented Apr 3, 2019 •
edited

Codecov Report

Merging #1405 into master will decrease coverage by 0.02%.
The diff coverage is 97.39%.

@@            Coverage Diff             @@
##           master    #1405      +/-   ##
==========================================
- Coverage   45.03%   45.01%   -0.03%     
==========================================
  Files         176      176              
  Lines       13042    13028      -14     
==========================================
- Hits         5874     5865       -9     
+ Misses       6818     6816       -2     
+ Partials      350      347       -3

@mjudeikis mjudeikis force-pushed the monitoring.noise branch 3 times, most recently from 5a90c42 to a3fd78a Compare April 3, 2019 10:44
@mjudeikis
Copy link
Contributor Author

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 3, 2019
@mjudeikis
Copy link
Contributor Author

/retest

@mjudeikis
Copy link
Contributor Author

@jim-minter If we could merge this, until we sort out https://github.com/openshift/openshift-azure/pull/1418/files . and when done we can move to monitor provisioning state as now it's a little bit broken.

jim-minter
jim-minter reviewed Apr 4, 2019
View reviewed changes
cmd/monitoring/monitoring.go Outdated
time.Sleep(10 * time.Second)
} else if iter.Value().IPAddress != nil {
hostnames = append(hostnames, *iter.Value().IPAddress)
break
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this break should be here

jim-minter
jim-minter reviewed Apr 4, 2019
View reviewed changes
cmd/monitoring/monitoring.go Outdated
break
}
}
if err == nil && len(hostnames) >= 4 {
Copy link
Contributor

Choose a reason for hiding this comment

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

"4" is very ugly

@jim-minter
Copy link
Contributor

force pushed
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 4, 2019
@jim-minter
Copy link
Contributor

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 4, 2019
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jim-minter, mjudeikis

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:
  • OWNERS [jim-minter,mjudeikis]

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

@openshift-bot
Copy link

/retest

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

2 similar comments
@openshift-bot
Copy link

/retest

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

@openshift-bot
Copy link

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 0ea45de into openshift:master Apr 4, 2019
@mjudeikis mjudeikis deleted the monitoring.noise branch April 26, 2019 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jim-minter jim-minter jim-minter left review comments

@y-cote y-cote Awaiting requested review from y-cote

Assignees

@jim-minter jim-minter

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. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
5 participants
@mjudeikis @jim-minter @openshift-ci-robot @openshift-bot @openshift-merge-robot