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
agnhost image: use actual DNS domain instead of hardcoded cluster.local #92964
agnhost image: use actual DNS domain instead of hardcoded cluster.local #92964
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. |
Welcome @vponomaryov! |
Hi @vponomaryov. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
This PR fixes following issue: #88752 |
4ab7aea
to
93089de
Compare
/retest |
@vponomaryov: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
Fixes #88752 |
/sig autoscaling |
/assign @claudiubelu @luxas |
/ok-to-test |
/lgtm |
/milestone v1.20 |
@jtslear: You must be a member of the kubernetes/milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your and have them propose you as an additional delegate for this responsibility. In response to this:
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. |
/assign @fejta |
/milestone v1.20 |
@justaugustus thanks for adding milestone label! [1] https://github.com/kubernetes/kubernetes/blob/master/test/images/agnhost/OWNERS |
Indeed, I didn't approve it since it also has changes outside of the agnhost image, so my approval wouldn't make this go in anyways. /approve |
cc @kubernetes/sig-network-pr-reviews |
/assign @thockin would like your $0.02 on the DNS changes here, especially w.r.t. #92964 (comment) and scraping/parsing search paths out of the resolv.conf file like this |
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.
LGTM, I just want this self-documenting
func getDNSSuffixList() []string { | ||
|
||
// GetDNSSuffixList reads DNS config file and returns the list of configured DNS suffixes | ||
func GetDNSSuffixList() []string { |
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.
FWIW using strings.Fields() would be safer WRT repeated whitespace
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.
The only thing that is changed here is that function becomes exported having uppercase first letter. So, I do not understand the point of the comment.
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.
exporting it and starting to use it as a utility method for other packages expands the scope of the code and increases the importance that it be robust/correct
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.
Strictly speaking this function gets a list of "search paths" and if I were to find "" in that list (which you would if there was a double-space) I would be very surprised.
Is there a test-case for this function? I'm not going to demand you go write one, unless you want to, but it probably should have a test including values with duplicate whitespace
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.
@liggitt , @thockin I agree that it would be good to fix this thing, but I disagree that it must be part of this particular change that has one concrete goal - fixing concrete bug. And the change you propose me doing is not related to the bugfix at all.
Then, about unit tests, the fact that I changed the name of the function and unit tests passed means that there are either no tests or they were not run. In anyway, it doesn't get tested on regular basis. So, it has much more TODOs which must be done, but definitely not as part of this change, IMHO.
) | ||
|
||
func getDNSDomain() string { |
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.
Can we please get a comment explaining how this is operating and why? It's PROBABLY safe, all things considered, but it's a heuristic.
This has come up in other places, and we really ought to expose it through downward API or something like that.
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.
Maybe it is good to have comments, but even this module has more complex functions without comments.
According to the activity on the bug-report which gets fixed here, no one really cared about it. So, I wonder exactly where it has come up?
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.
Past mistakes do not mean we shouldn't do better. The function is sufficiently subtle that having an explanation of what it is supposed to be doing and why is justified. I'm not asking for a book, just a couple sentences. Less than this comment, probably.
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.
Added comment in the code here and in the place of it's call.
@@ -214,7 +240,7 @@ func (c *controller) sendConsumeCustomMetric(w http.ResponseWriter, metric strin | |||
} | |||
|
|||
func createConsumerURL(suffix string) string { | |||
return fmt.Sprintf("http://%s.%s.svc.cluster.local:%d%s", consumerServiceName, consumerServiceNamespace, consumerPort, suffix) |
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.
WRT Windows, this is one of a long list of things that don't work right in Windows which were deemed "acceptable because no Windows user would expect them to work", even if a Kube user would. Sigh.
test/images/agnhost/agnhost.go
Outdated
@@ -51,7 +51,7 @@ import ( | |||
func main() { | |||
rootCmd := &cobra.Command{ | |||
Use: "app", | |||
Version: "2.22", | |||
Version: "2.23", |
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.
Why do we need to update the version in 3 places? Can't we inject it at build time via linker?
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.
I agree that it is not ok to have such kind of duplication, but I do not implement it here. I just do minimal changes needed to fix the issue.
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.
sure, that's fine - I don't know who owns this sub-component now, so I guess I hope they see this and agree.
da7d8b7
to
754331b
Compare
I've always wondered why the cluster domain isn't available through the downward API. |
@thockin are the added description comments look good for you? |
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.
/approve
the build changes
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: claudiubelu, fejta, vponomaryov 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 |
'agnhost' image uses hardcoded 'cluster.local' value for DNS domain. It leads to failure of a bunch of HPA tests when test cluster is configured to use custom DNS domain and there is no alias for default 'cluster.local' one. So, fix it by reusing it's own function for reading DNS domain suffixes. Signed-off-by: Valerii Ponomarov <kiparis.kh@gmail.com>
754331b
to
c55b6cd
Compare
/lgtm |
'agnhost' image uses hardcoded 'cluster.local' value for DNS domain.
It leads to failure of a bunch of HPA tests when test cluster is
configured to use custom DNS domain and there is no alias for
default 'cluster.local' one.
So, fix it by reusing it's own function for reading DNS domain suffixes.
Bumps 'agnhost' image version to 2.24
Signed-off-by: Valerii Ponomarov kiparis.kh@gmail.com
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: