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

Adding V3 compatible windows integration #744

Closed
wants to merge 17 commits into from
Closed

Adding V3 compatible windows integration #744

wants to merge 17 commits into from

Conversation

RagalahariP
Copy link

@RagalahariP RagalahariP commented May 8, 2023 •
edited
Loading

  • Adding windows-daemonset.yaml file similar to V2 and supporting changes in templates.

Testing : Tested in local EKS cluster with the following node config

kubectl get nodes -o wide
NAME                          STATUS   ROLES    AGE     VERSION                INTERNAL-IP   EXTERNAL-IP      OS-IMAGE                         KERNEL-VERSION                 CONTAINER-RUNTIME
ip-10-0-11-231.ec2.internal   Ready    <none>   3d21h   v1.23.17-eks-a59e1f0   10.0.11.231   18.208.247.135   Windows Server 2022 Datacenter   10.0.20348.1668                docker://20.10.21
ip-10-0-110-39.ec2.internal   Ready    <none>   22d     v1.23.17-eks-a59e1f0   10.0.110.39   <none>           Windows Server 2019 Datacenter   10.0.17763.4252                docker://20.10.21
ip-10-0-27-190.ec2.internal   Ready    <none>   22d     v1.23.17-eks-a59e1f0   10.0.27.190   3.95.250.93      Amazon Linux 2                   5.4.238-148.347.amzn2.x86_64   docker://20.10.17

Dashboard: https://onenr.io/0BQ18La75jx

@RagalahariP RagalahariP requested a review from a team May 8, 2023 17:27
@CLAassistant
Copy link

CLAassistant commented May 8, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@RagalahariP RagalahariP force-pushed the V3-windows-support branch 2 times, most recently from f55ab87 to d1fb110 Compare May 9, 2023 17:57
nrepai
nrepai reviewed May 9, 2023
View reviewed changes
charts/newrelic-infrastructure/templates/clusterrole.yaml Outdated Show resolved Hide resolved
charts/newrelic-infrastructure/templates/kubelet/daemonset.yaml Show resolved Hide resolved
charts/newrelic-infrastructure/values-windows-linux.yaml Outdated Show resolved Hide resolved
charts/newrelic-infrastructure/values-windows-linux.yaml Outdated Show resolved Hide resolved
charts/newrelic-infrastructure/templates/kubelet/daemonset-windows.yaml Show resolved Hide resolved
charts/newrelic-infrastructure/values-windows-linux.yaml Outdated
licenseKey: <>
cluster: <>

kubelet:
Copy link

Choose a reason for hiding this comment

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

should this be in values.yaml? i don't see it being imported anywhere

charts/newrelic-infrastructure/templates/kubelet/daemonset-windows.yaml Outdated Show resolved Hide resolved
@RagalahariP RagalahariP changed the title (Draft)Adding V3 compatible windows integration Adding V3 compatible windows integration May 12, 2023
@RagalahariP RagalahariP requested a review from nrepai May 12, 2023 15:54
nrepai
nrepai approved these changes May 12, 2023
View reviewed changes
Copy link

@nrepai nrepai left a comment

Choose a reason for hiding this comment

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

lgtm. i dont have merge permission for this repo but also we should get a second pair of eyes on this. ill ping the team

charts/newrelic-infrastructure/values-windows.yaml Outdated
# Reference yaml for Windows V3 integration

global:
licenseKey: <>
Copy link

Choose a reason for hiding this comment

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

are those needed? i imagine we can get the keys from values.yaml

Copy link
Contributor

Choose a reason for hiding this comment

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

@RagalahariP can you address this comment? I'm not sure this is needed.

Copy link
Author

Choose a reason for hiding this comment

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

@htroisi Yes, I have deleted redundant values-windows.yaml . Now values.yaml has windows config option.

@nrepai
Copy link

nrepai commented May 12, 2023

actually i just noticed the tests are failing. not sure if it's specific to this branch since I see other PRs failing

htroisi
htroisi reviewed May 12, 2023
View reviewed changes
charts/newrelic-infrastructure/values-windows.yaml Outdated
cluster: <>

kubelet:
enableWindows: true
Copy link
Contributor

@htroisi htroisi May 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Should we upstream enableWindows and windowsOsList in the chart's values.yaml and README file?

Copy link
Author

Choose a reason for hiding this comment

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

Updated

@RagalahariP RagalahariP requested a review from htroisi May 15, 2023 17:55
@htroisi
Copy link
Contributor

htroisi commented May 16, 2023

@RagalahariP I DM'd you the instructions to get the E2E tests to pass.

htroisi
htroisi reviewed May 16, 2023
View reviewed changes
charts/newrelic-infrastructure/values.yaml Outdated
@@ -130,6 +130,16 @@ kubelet:
scraperMaxReruns: 4
# port:
# scheme:
# Set enableWindows to true to enable monitoring on Windows nodes
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you modify the comments for enableWindows and enableWindows to use the same formatting as the rest of the file?
Once you've modified the formatting can you run helm-docs tool to automatically update the README.md file?

Copy link
Author

Choose a reason for hiding this comment

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

Updated with helm-docs

@RagalahariP RagalahariP mentioned this pull request May 18, 2023
Open
@htroisi
Copy link
Contributor

htroisi commented May 19, 2023

Closing this PR is favor of #754 since E2E tests don't work from forks.

@htroisi htroisi closed this May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htroisi htroisi Awaiting requested review from htroisi

@nrepai nrepai Awaiting requested review from nrepai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RagalahariP @CLAassistant @nrepai @htroisi