set mellanox reg key #1768

rajvinar · 2023-01-24T20:28:23Z

Reason for Change:

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests

Notes:

rbtr

This seems like underlying infra config, and I'm not sure it should be CNS's job to configure prerequisites like this. Shouldn't this responsibility lie with the orchestrator which expects to use CNS/CNI?

cns/service/main.go

thatmattlong · 2023-01-26T23:06:48Z

Does mellanox driver require reboot? I agree with @rbtr that we probably shouldn't be setting this in CNS, but if it requires a reboot then we definitely cannot set this with CNS.

rajvinar · 2023-01-26T23:12:19Z

This seems like underlying infra config, and I'm not sure it should be CNS's job to configure prerequisites like this. Shouldn't this responsibility lie with the orchestrator which expects to use CNS/CNI?

This script/code to set PriorityVLANTag already been added to
Atlas code (https://msazure.visualstudio.com/One/_git/SF-SeaBreeze?path=%2Fsrc%2Fscripts%2FAtlasVMSetup.20220112.ps1&_a=contents&version=GBmain)

but it seems it gets executed at starting of cluster but eventually value get reset and its happening very often(during live migration for which we have no control) so we want to make it part of CNS to make sure mellanox adapter's value is always set.

rajvinar · 2023-01-26T23:23:51Z

Does mellanox driver require reboot? I agree with @rbtr that we probably shouldn't be setting this in CNS, but if it requires a reboot then we definitely cannot set this with CNS.

yes for version 3 it requires 'reboot adapter only not vm'. Let me confirm with Paul again. May I know why we cannot execute this in CNS if it requires rebooting adapter. Just curious.

rajvinar · 2023-01-26T23:45:30Z

@microsoft-github-policy-service agree [company="{your company}"]
@microsoft-github-policy-service agree company="Microsoft"

rajvinar · 2023-01-26T23:46:58Z

@microsoft-github-policy-service agree company="Microsoft"

@microsoft-github-policy-service agree company="Microsoft"

pjohnst5 · 2023-01-27T18:48:07Z

This seems like underlying infra config, and I'm not sure it should be CNS's job to configure prerequisites like this. Shouldn't this responsibility lie with the orchestrator which expects to use CNS/CNI?

That's a great point, we have tried this, but the setting gets un-set eventually, either due to live migration of the node underneath the VM, or otherwise, so we can't rely on it being set once and done, we'll have to keep setting it in CNS to make sure

pjohnst5 · 2023-01-27T18:48:37Z

And VM hosting team is unwilling / unable to set it to the values we need by default across Azure
Vivek Bhanu is his name

pjohnst5 · 2023-01-27T18:49:14Z

Does mellanox driver require reboot? I agree with @rbtr that we probably shouldn't be setting this in CNS, but if it requires a reboot then we definitely cannot set this with CNS.

A reboot of the network adapter is needed on one older version Mellanox x3

The VM does not need to be rebooted

pjohnst5 · 2023-01-31T19:24:50Z

Hi @ashvindeodhar , I was thinking to put the setmellanoxadapter function call into the CreateorUpdateNetworkContainer
Reason being, we would hit the same issue here, of live migration (or any other reason that the priorityvlantag gets reset by something out of our stack), failing the datapath

If we set the mellanox adapter each time, then there's a chance when the priorityvlantag is reset by some live-migration or anything else, createNC call would fix it right away, instead of restarting CNS

cns/service/main.go

platform/os_windows.go

ashvindeodhar · 2023-01-31T23:17:34Z

Hi @ashvindeodhar , I was thinking to put the setmellanoxadapter function call into the CreateorUpdateNetworkContainer Reason being, we would hit the same issue here, of live migration (or any other reason that the priorityvlantag gets reset by something out of our stack), failing the datapath

If we set the mellanox adapter each time, then there's a chance when the priorityvlantag is reset by some live-migration or anything else, createNC call would fix it right away, instead of restarting CNS

@pjohnst5 yes this is why I had added the sdn arp mac reg key at the time of getNC call to CNS. a1f804a @tamilmani1989 any idea why that was moved to main func in CNS?

tamilmani1989 · 2023-02-01T00:14:32Z

Does setting this registry key affect singletenancy scenarios? I see its set for all scenarios.

tamilmani1989 · 2023-02-01T00:27:08Z

The downside of setting this in CNI and not by orchestrator bootup script is that if in future something changes like registry value changes for different version of Mellanox, then we have to deal with supporting all versions based on what used.

tamilmani1989 · 2023-02-01T00:29:52Z

Hi @ashvindeodhar , I was thinking to put the setmellanoxadapter function call into the CreateorUpdateNetworkContainer Reason being, we would hit the same issue here, of live migration (or any other reason that the priorityvlantag gets reset by something out of our stack), failing the datapath
If we set the mellanox adapter each time, then there's a chance when the priorityvlantag is reset by some live-migration or anything else, createNC call would fix it right away, instead of restarting CNS

@pjohnst5 yes this is why I had added the sdn arp mac reg key at the time of getNC call to CNS. a1f804a @tamilmani1989 any idea why that was moved to main func in CNS?

putting in createorupdatenc indirectly dependent on CNI to execute sequentially. Now that we removed lock acquiring at beginning of cni process, it can execute parallel which may call setting registry key multiple times which leads to hns restarts multiple times

rbtr · 2023-02-01T20:06:53Z

I remain opposed to this kind of underlying infrastructure configuration being CNS's responsibility at all.

However, if it were, it must be in a startup/preflight and guaranteed to only execute once per process. If the config becomes unset such that we can't do our job anymore, we should crash out so that it is reset during reinitialization.

This seems like underlying infra config, and I'm not sure it should be CNS's job to configure prerequisites like this. Shouldn't this responsibility lie with the orchestrator which expects to use CNS/CNI?

That's a great point, we have tried this, but the setting gets un-set eventually, either due to live migration of the node underneath the VM, or otherwise, so we can't rely on it being set once and done, we'll have to keep setting it in CNS to make sure

That some registry flags get unset during a live migration kinda sounds like a bug with live migration, no? And if something else is randomly resetting the value, we need to understand who and why and ask them to stop, maybe? Otherwise we just fight over the reg key.

pjohnst5 · 2023-02-02T18:52:51Z

Rajvi is following up with Vivek Bhanu and Michael Nanakul as to why the registry key is reset in the first place

ramiro-gamarra

Some early feedback, not exhaustive. Please feel free to set some time up if you'd like to discuss

platform/os_windows.go

rbtr · 2023-03-28T20:28:26Z

@rajvinar are you not a member of our GitHub team? You will need to join that so that the workflows will run.

rajvinar · 2023-03-28T22:22:41Z

@rajvinar are you not a member of our GitHub team? You will need to join that so that the workflows will run.

ohh let me do that.. thanks for info

approved by Ramiro

pjohnst5

Not really requesting changes, but want to block this until we have 1 more question cleared up

Question is, should we only run this, if config.ChannelMode == cns.Direct ?

Reason I ask is, this is only a known issue for multitenant orchestrators (using windows)

AKS doesn't need this change (unless I'm missing something, where they do have multitenant windows scenarios)

So perhaps better to only run this if cns is in direct mode, where we know the issue arises

pjohnst5 · 2023-06-13T18:15:32Z

Yeah so I confirmed with @thatmattlong offline that AKS doesn't use multitenant windows scenarios, so they don't need this change

So the only confirmed place we need this is if config.ChannelMode == cns.Direct, I think we should add that 'if'

pjohnst5

Discussed this offline with @ashvindeodhar and @ramiro-gamarra , that it is better to add this only for cns.Direct mode, to scope it only where we've seen the problem, and leave other scenarios (who don't need this fix), untouched, to avoid any unforseen issues with other scenarios

(cherry picked from commit fa2de6d)

* build azure-vnet-telemetry and azure-vnet-ipam in dropgz-test (#1846) build azure-vnet-telemetry and azure-vnet-ipam in dropgz-test for parity with release image Signed-off-by: Evan Baker <rbtr@users.noreply.github.com> (cherry picked from commit f619259) * ci: disable kube-proxy for test clusters (#1965) * disable kube-proxy for byocni cluster creation * test config mapping * shell pwd * use CURDIR * check current directory * test with repo root dir * test azp format * test azp format * test azp format * change e2e steps to remove kube proxy * fix load test update args * fix ns and rg in update * update ciliume2e * fix kubectl cmd in load test * adding new targets for no kube proxy * remove cluster update * update overlay e2e * test behavior of load test * test grep for azure-cns * look for container deployment * testing * restart node variable check * update if condition * add skip node case --------- Co-authored-by: tamilmani1989 <tamanoha@microsoft.com> (cherry picked from commit 024819d) * CI: [CNI] Replace the bash scripts for CNI load testing with golang test cases (#2003) CI:[CNI] Replace the bash scripts with the golang test cases (cherry picked from commit 008ae45) * ci: [CNI] Move Nightly Cilium Pipeline test to ACN (#1963) * CNS to be able to generate dualstack overaly CNI conflist (#1981) * fix: Eliminating duplicate lines * ci: Add update permission for ciliumidentity * fix: Parameterize Image Registry add retry to nnc update during scaledown (#1970) * add retry to nnc update during scaledown Signed-off-by: Evan Baker <rbtr@users.noreply.github.com> * test for panic in pool monitor Signed-off-by: Evan Baker <rbtr@users.noreply.github.com> --------- Signed-off-by: Evan Baker <rbtr@users.noreply.github.com> fix: reserve 0th IP as gateway for overlay on Windows (#1968) * fix: reserve 0th IP as gateway for overlay on Windows * fix: allow gateway to be updated ci: windows profile container image (#1988) Always use 0 for NC version in Overlay (#1979) always use 0 for NC version in overlay Signed-off-by: Evan Baker <rbtr@users.noreply.github.com> [Vnet Scale - CNS]: Flattening CIDR ranges for Node NNC to a list (#1921) * Read secondary CIDRs from VnetScale NNC * fix comment * update comment * For VnetScale mode, Use 1st IP for def gateway instead of 0th for windows * fix/add import * address pr comments * add comments * address pr comments * wrap error * fix typo * fix UT fix: [NPM] check if policy exists in case of nil pointer (#1974) fix: check for nil first ci: disable kube-proxy for test clusters (#1965) * disable kube-proxy for byocni cluster creation * test config mapping * shell pwd * use CURDIR * check current directory * test with repo root dir * test azp format * test azp format * test azp format * change e2e steps to remove kube proxy * fix load test update args * fix ns and rg in update * update ciliume2e * fix kubectl cmd in load test * adding new targets for no kube proxy * remove cluster update * update overlay e2e * test behavior of load test * test grep for azure-cns * look for container deployment * testing * restart node variable check * update if condition * add skip node case --------- Co-authored-by: tamilmani1989 <tamanoha@microsoft.com> perf: [WIN-NPM] fast bootup (#1900) * wip * wip2 * use other apply DP func * address comment about if statement * finish bootup for both DPs * fix lint * fix lint 2 * fix lint 3 * longer UT timeout and add missing UTs for apply in background tool: [NPM] script to clean up iptable chains (#1978) tool: script to clean up NPM iptable chains feat: [WIN-NPM] metrics for latencies and failures (#1959) * implement metrics * add npm prefix * rename windows files * metrics pkg UTs * allow reinitializing prometheus metrics * fix: hns wrapper should not throw error for empty SetPolicy values * test: metric UTs in dataplane * fix: record list endpoint latency always * remove flaky UT * feat: metric for max ipset members * fix lint * fix lint 2 * fix build * fix lint 3 * simplify conditionals and protect against maxMembers becoming negative * remove bottom 4 histogram buckets. start at 16 ms * reset metrics for ipset UTs * style: don't check for windows dp in *_windows.go files * build: remove unused import * test: reset windows metrics in UT Remove SSH port 22 rule from aks-engine clusters (#1983) ci: change overlaye2e stage to cilium-overlay (#1997) * renaming overlaye2e for cilium * update display names for stages Initial getHomeAZ 404 changes (#1994) * initial getHomeAZ 404 changes * treat 404 as success * address comments CNS to be able to generate dualstack overaly CNI conflist (#1981) fix: Parameterize Image Registry add retry to nnc update during scaledown (#1970) * add retry to nnc update during scaledown Signed-off-by: Evan Baker <rbtr@users.noreply.github.com> * test for panic in pool monitor Signed-off-by: Evan Baker <rbtr@users.noreply.github.com> --------- Signed-off-by: Evan Baker <rbtr@users.noreply.github.com> fix: reserve 0th IP as gateway for overlay on Windows (#1968) * fix: reserve 0th IP as gateway for overlay on Windows * fix: allow gateway to be updated ci: windows profile container image (#1988) Always use 0 for NC version in Overlay (#1979) always use 0 for NC version in overlay Signed-off-by: Evan Baker <rbtr@users.noreply.github.com> [Vnet Scale - CNS]: Flattening CIDR ranges for Node NNC to a list (#1921) * Read secondary CIDRs from VnetScale NNC * fix comment * update comment * For VnetScale mode, Use 1st IP for def gateway instead of 0th for windows * fix/add import * address pr comments * add comments * address pr comments * wrap error * fix typo * fix UT fix: [NPM] check if policy exists in case of nil pointer (#1974) fix: check for nil first ci: disable kube-proxy for test clusters (#1965) * disable kube-proxy for byocni cluster creation * test config mapping * shell pwd * use CURDIR * check current directory * test with repo root dir * test azp format * test azp format * test azp format * change e2e steps to remove kube proxy * fix load test update args * fix ns and rg in update * update ciliume2e * fix kubectl cmd in load test * adding new targets for no kube proxy * remove cluster update * update overlay e2e * test behavior of load test * test grep for azure-cns * look for container deployment * testing * restart node variable check * update if condition * add skip node case --------- Co-authored-by: tamilmani1989 <tamanoha@microsoft.com> perf: [WIN-NPM] fast bootup (#1900) * wip * wip2 * use other apply DP func * address comment about if statement * finish bootup for both DPs * fix lint * fix lint 2 * fix lint 3 * longer UT timeout and add missing UTs for apply in background tool: [NPM] script to clean up iptable chains (#1978) tool: script to clean up NPM iptable chains feat: [WIN-NPM] metrics for latencies and failures (#1959) * implement metrics * add npm prefix * rename windows files * metrics pkg UTs * allow reinitializing prometheus metrics * fix: hns wrapper should not throw error for empty SetPolicy values * test: metric UTs in dataplane * fix: record list endpoint latency always * remove flaky UT * feat: metric for max ipset members * fix lint * fix lint 2 * fix build * fix lint 3 * simplify conditionals and protect against maxMembers becoming negative * remove bottom 4 histogram buckets. start at 16 ms * reset metrics for ipset UTs * style: don't check for windows dp in *_windows.go files * build: remove unused import * test: reset windows metrics in UT Remove SSH port 22 rule from aks-engine clusters (#1983) ci: change overlaye2e stage to cilium-overlay (#1997) * renaming overlaye2e for cilium * update display names for stages Initial getHomeAZ 404 changes (#1994) * initial getHomeAZ 404 changes * treat 404 as success * address comments CNS to be able to generate dualstack overaly CNI conflist (#1981) * fix: File Directory * style: Comments * Addressing Comments --------- Co-authored-by: Paul Johnston <35265851+pjohnst5@users.noreply.github.com> (cherry picked from commit 1514d95) * ci:[CNI] Add windows CNIv1 datapath test (#2016) * ci: Transfer files * test: Working Datapath Test * test: apierror Tests * style: Datapath Package * test: Deployment timing * fix: Error check * fix: Lint (cherry picked from commit 390977d) * fix: [CNI] CNI load test failing due to namespace already created (#2031) fix: CNI load test failing due to namespace already created (cherry picked from commit c10900e) * ci:[CNI] Windows cniv1 load test pipeline (#2024) CI:[CNI] Windows cniv1 load test pipeline (cherry picked from commit e45ad21) * ci: [CNI] Adding aks cluster creation steps for k8s e2e test (#2052) * ci: [CNI] Adding aks cluster creation steps for k8s e2e test * Add validate step to the pipeline * Adding the telemetry config to the cluster (cherry picked from commit 846e508) * ci:[CNI] Replace AKS-Engine Tests with k8s conformance tests (#2062) * Initial Commit * Add attempts to prevent flakyness * Add taint for windows tests * Add k8s e2e tests * Testing vmSizes * Artifact k8se2e binary * Remove NPM E2E * Add testing and increase processes * Addressing comments (cherry picked from commit 451c691) * CI: Removing AKS engine related code (#2089) (cherry picked from commit b45c2c7) * feat: [dropgz] Dropgz for windows (#2075) * feat: [dropgz] Dropgz for windows * Removing the code for killing the process from dropgz for windows (cherry picked from commit 7a41178) * ci: Update dns tests for k8s conformance (#2104) Update dns tests for k8s v1.26 (cherry picked from commit bbf2fd4) * ci: adding cni package as a trigger (#2108) (cherry picked from commit e6a8ea6) * ci: add packages for submodule trigger (#2154) (cherry picked from commit 4aecfd6) * set mellanox reg key (#1768) (cherry picked from commit fa2de6d) --------- Co-authored-by: Evan Baker <rbtr@users.noreply.github.com> Co-authored-by: Camryn Lee <31013536+camrynl@users.noreply.github.com> Co-authored-by: Vipul Singh <vipul21sept@gmail.com> Co-authored-by: Rajvi <107083915+rajvinar@users.noreply.github.com>

rbtr reviewed Jan 24, 2023

View reviewed changes

cns/service/main.go Outdated Show resolved Hide resolved

rajvinar force-pushed the Rajvi-SetMellanoxRegKey branch from e71061b to 4f2b60d Compare January 26, 2023 02:02

ashvindeodhar requested changes Jan 26, 2023

View reviewed changes

rajvinar changed the title ~~Rajvi set mellanox reg key~~ set mellanox reg key Jan 26, 2023

ashvindeodhar previously requested changes Jan 31, 2023

View reviewed changes

rajvinar force-pushed the Rajvi-SetMellanoxRegKey branch 2 times, most recently from 94ee01c to bb2b7d6 Compare February 22, 2023 18:53

ramiro-gamarra reviewed Feb 22, 2023

View reviewed changes

rajvinar marked this pull request as ready for review March 28, 2023 18:15

rajvinar requested a review from a team as a code owner March 28, 2023 18:15

rajvinar requested review from rbtr and removed request for a team March 28, 2023 18:15

rajvinar force-pushed the Rajvi-SetMellanoxRegKey branch from e19af37 to e9e7a76 Compare March 28, 2023 18:15

rajvinar added 8 commits June 12, 2023 10:34

lint changes

471c284

lint changes

4bde126

lint changes

6675f79

lint changes

760708a

lint changes

59021a1

lint changes

97e9a61

change to fix %q bug which cause escape when not needed

60c2bb5

lint changes

1910312

rajvinar force-pushed the Rajvi-SetMellanoxRegKey branch from dffeac0 to 1910312 Compare June 12, 2023 17:36

rajvinar requested a review from timraymond June 12, 2023 18:04

Merge branch 'master' into Rajvi-SetMellanoxRegKey

9728ef8

pjohnst5 self-requested a review June 12, 2023 23:17

pjohnst5 requested changes Jun 12, 2023

View reviewed changes

If config.channelmode == cns.Direct only, do we want this

904097e

pjohnst5 dismissed ramiro-gamarra’s stale review via 904097e June 13, 2023 18:53

pjohnst5 approved these changes Jun 13, 2023

View reviewed changes

ramiro-gamarra approved these changes Jun 13, 2023

View reviewed changes

pjohnst5 enabled auto-merge (squash) June 13, 2023 19:30

pjohnst5 merged commit fa2de6d into Azure:master Jun 13, 2023
37 checks passed

jpayne3506 pushed a commit that referenced this pull request Sep 12, 2023

set mellanox reg key (#1768)

968d166

(cherry picked from commit fa2de6d)

jpayne3506 pushed a commit that referenced this pull request Sep 12, 2023

set mellanox reg key (#1768)

457b9f1

(cherry picked from commit fa2de6d)

jpayne3506 pushed a commit that referenced this pull request Sep 12, 2023

set mellanox reg key (#1768)

ab853b8

(cherry picked from commit fa2de6d)

jpayne3506 pushed a commit that referenced this pull request Sep 12, 2023

set mellanox reg key (#1768)

8cf0439

(cherry picked from commit fa2de6d)

jpayne3506 pushed a commit that referenced this pull request Sep 17, 2023

set mellanox reg key (#1768)

ded7fdd

(cherry picked from commit fa2de6d)

jpayne3506 pushed a commit that referenced this pull request Sep 18, 2023

set mellanox reg key (#1768)

9e1fe4a

(cherry picked from commit fa2de6d)

jpayne3506 pushed a commit that referenced this pull request Sep 20, 2023

set mellanox reg key (#1768)

f25f01d

(cherry picked from commit fa2de6d)

smittal22 mentioned this pull request Jul 25, 2024

Fix condition to use default value in case parameter is not set #2880

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set mellanox reg key #1768

set mellanox reg key #1768

rajvinar commented Jan 24, 2023

rbtr left a comment

thatmattlong commented Jan 26, 2023

rajvinar commented Jan 26, 2023 •

edited

Loading

rajvinar commented Jan 26, 2023 •

edited

Loading

rajvinar commented Jan 26, 2023 •

edited

Loading

rajvinar commented Jan 26, 2023

pjohnst5 commented Jan 27, 2023

pjohnst5 commented Jan 27, 2023

pjohnst5 commented Jan 27, 2023

pjohnst5 commented Jan 31, 2023

ashvindeodhar commented Jan 31, 2023

tamilmani1989 commented Feb 1, 2023

tamilmani1989 commented Feb 1, 2023

tamilmani1989 commented Feb 1, 2023

rbtr commented Feb 1, 2023

pjohnst5 commented Feb 2, 2023

ramiro-gamarra left a comment

rbtr commented Mar 28, 2023

rajvinar commented Mar 28, 2023

pjohnst5 left a comment

pjohnst5 commented Jun 13, 2023

pjohnst5 left a comment

set mellanox reg key #1768

set mellanox reg key #1768

Conversation

rajvinar commented Jan 24, 2023

rbtr left a comment

Choose a reason for hiding this comment

thatmattlong commented Jan 26, 2023

rajvinar commented Jan 26, 2023 • edited Loading

rajvinar commented Jan 26, 2023 • edited Loading

rajvinar commented Jan 26, 2023 • edited Loading

rajvinar commented Jan 26, 2023

pjohnst5 commented Jan 27, 2023

pjohnst5 commented Jan 27, 2023

pjohnst5 commented Jan 27, 2023

pjohnst5 commented Jan 31, 2023

ashvindeodhar commented Jan 31, 2023

tamilmani1989 commented Feb 1, 2023

tamilmani1989 commented Feb 1, 2023

tamilmani1989 commented Feb 1, 2023

rbtr commented Feb 1, 2023

pjohnst5 commented Feb 2, 2023

ramiro-gamarra left a comment

Choose a reason for hiding this comment

rbtr commented Mar 28, 2023

rajvinar commented Mar 28, 2023

pjohnst5 left a comment

Choose a reason for hiding this comment

pjohnst5 commented Jun 13, 2023

pjohnst5 left a comment

Choose a reason for hiding this comment

rajvinar commented Jan 26, 2023 •

edited

Loading

rajvinar commented Jan 26, 2023 •

edited

Loading

rajvinar commented Jan 26, 2023 •

edited

Loading