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

For Windows, the hostname in kube-apiserver is in lowercase. #2672

Merged
merged 1 commit into from Sep 1, 2021

Conversation

shettyg
Copy link
Contributor

@shettyg shettyg commented Aug 28, 2021

Currently, the startup scripts force an environment variable
to have lowercase node name. This gets complex once we add
support to native windows services as they don't get
env variables from local shell.

An example of startup script is function: Start-AntreaAgent in
https://raw.githubusercontent.com/antrea-io/antrea/v1.2.0/hack/windows/Helper.psm1

Signed-off-by: Gurucharan Shetty gurushetty@google.com

wenyingd
wenyingd previously approved these changes Aug 30, 2021
Copy link
Contributor

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

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

LGTM

lzhecheng
lzhecheng previously approved these changes Aug 30, 2021
Copy link
Contributor

@lzhecheng lzhecheng left a comment

Choose a reason for hiding this comment

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

LGTM.
/test-all

@lzhecheng
Copy link
Contributor

/test-all

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2021

Codecov Report

Merging #2672 (7923400) into main (d8d70ac) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2672   +/-   ##
=======================================
  Coverage   60.70%   60.71%           
=======================================
  Files         285      285           
  Lines       23003    23006    +3     
=======================================
+ Hits        13965    13967    +2     
+ Misses       7544     7540    -4     
- Partials     1494     1499    +5     
Flag Coverage Δ
e2e-tests ∅ <ø> (?)
kind-e2e-tests 48.41% <0.00%> (+0.03%) ⬆️
unit-tests 41.06% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/util/env/env.go 41.46% <0.00%> (-2.13%) ⬇️
pkg/controller/grouping/controller.go 67.24% <0.00%> (-1.73%) ⬇️
pkg/agent/flowexporter/exporter/exporter.go 80.23% <0.00%> (-1.17%) ⬇️
pkg/agent/cniserver/server.go 68.91% <0.00%> (-0.65%) ⬇️
pkg/agent/util/net.go 35.82% <0.00%> (-0.55%) ⬇️
pkg/agent/route/route_linux.go 43.50% <0.00%> (-0.50%) ⬇️
pkg/agent/openflow/pipeline.go 75.23% <0.00%> (+0.07%) ⬆️
pkg/legacyclient/clientset/versioned/clientset.go 30.64% <0.00%> (+4.83%) ⬆️
pkg/controller/networkpolicy/status_controller.go 77.12% <0.00%> (+5.88%) ⬆️

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

The Node name is in lowercase on Linux too. kubelet converts it when reporting nodeName regardless of the platform, and server side API validation requires all metadata.name are in lower case.
So could we adjust the commit title a little to avoid confusion? e.g. "Convert NodeName got from env var to lowercase on Windows"

Currently, the startup scripts force an environment variable
to have lowercase node name. This gets complex once we add
support to native windows services as they don't get
env variables from local shell.

An example of startup script is function: Start-AntreaAgent in
https://raw.githubusercontent.com/antrea-io/antrea/v1.2.0/hack/windows/Helper.psm1

Signed-off-by: Gurucharan Shetty <gurushetty@google.com>
@shettyg
Copy link
Contributor Author

shettyg commented Aug 31, 2021

The Node name is in lowercase on Linux too. kubelet converts it when reporting nodeName regardless of the platform, and server side API validation requires all metadata.name are in lower case.
So could we adjust the commit title a little to avoid confusion? e.g. "Convert NodeName got from env var to lowercase on Windows"

I updated the commit subject. Please take a look again

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@tnqn
Copy link
Member

tnqn commented Aug 31, 2021

/test-all
/test-windows-all

@antoninbas
Copy link
Contributor

@tnqn feel free to cherry-pick this to release-1.3 if you think it is a good idea

@tnqn
Copy link
Member

tnqn commented Aug 31, 2021

/skip-e2e as "TestLegacyAntreaPolicy/TestGroupAuditLogging" failed for known reason and has been fixed in #2683.
/test-conformance

@tnqn
Copy link
Member

tnqn commented Aug 31, 2021

/test-windows-all

@tnqn
Copy link
Member

tnqn commented Aug 31, 2021

/test-windows-e2e
/test-windows-networkpolicy
/test-windows-conformance

@tnqn
Copy link
Member

tnqn commented Aug 31, 2021

/test-windows-networkpolicy
/test-windows-conformance

@tnqn
Copy link
Member

tnqn commented Sep 1, 2021

jenkins-windows-networkpolicy failed due to some certificate issue. And apparently the change is not the cause. Merging it as other windows tests succeeded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/backport Indicates a PR that requires backports.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants