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
connectivitycheckcontroller: name endpoints using node names #915
connectivitycheckcontroller: name endpoints using node names #915
Conversation
/test e2e-aws-serial |
syncContext.Recorder().Warningf("EndpointDetectionFailure", "%s: %v", resourcehelper.FormatResourceForCLIWithNamespace(check), err) | ||
continue | ||
} | ||
syncContext.Recorder().Eventf("EndpointCheckCreated", "Updated %s because it changed.", resourcehelper.FormatResourceForCLIWithNamespace(check)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EndpointCheckUpdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syncContext.Recorder().Warningf("EndpointDetectionFailure", "%s: %v", resourcehelper.FormatResourceForCLIWithNamespace(check), err) | ||
continue | ||
} | ||
syncContext.Recorder().Eventf("EndpointCheckCreated", "Updated %s because it changed.", resourcehelper.FormatResourceForCLIWithNamespace(check)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these events are going to the wrong namespace. We want them in the TargetNamespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these events are going to the wrong namespace. We want them in the
TargetNamespace
will take a bug to fix it instead of fixing it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these events are going to the wrong namespace. We want them in the
TargetNamespace
@deads2k Are you sure? These are events from the operator, managing the Spec
of the PodNetworkConnectivityChecks (similar to events creating other resources in target namespace). Compared this to the events coming from the check-endpoints tool that runs in the target namespace and updates the Status
of the PodNetworkConnectivityChecks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it wasn't where I looked for them, but I guess I can live with it now that I know.
some comments, but this is a strict improvement /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, sanchezl 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 |
/retest Please review the full test history for this PR and help us cut down flakes. |
8 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
Create node "scoped" connectivity checks who's source pod and endpoints can change over time. Requires #914.