Skip to content

Conversation

@smittal22
Copy link
Contributor

@smittal22 smittal22 commented Jan 30, 2023

Reason for Change:

To enable AZR support, we need to pass two extra params azID (uint) and enableAZR (boolean) as part of body to NMAgent. Since we are not passing body to NMAgent as of now, this PR adds a body to the request just like we have for publish NC.

Whenever NC has enableAZR = true in NC config, we will pass body like: {"azID":<value>,"enableAZR":true}
For already existing NC (or if NC has enableAZR = false in NC config), we will pass body like: {"azID": 0,"enableAZR":false}

Issue Fixed:

https://msazure.visualstudio.com/DefaultCollection/One/_workitems/edit/16187805

Requirements:

Notes: e2e flow has been tested by creating and deleting NCs with both enableAZR set to true and false

@smittal22 smittal22 force-pushed the saksham_azr_nc_delete branch from 91d6826 to 9691d9c Compare January 30, 2023 20:58
@smittal22 smittal22 marked this pull request as ready for review January 30, 2023 23:04
@smittal22 smittal22 requested a review from a team as a code owner January 30, 2023 23:04
@smittal22 smittal22 requested review from rbtr and removed request for a team January 30, 2023 23:04
@smittal22 smittal22 changed the title [WIP] Initial delete NC changes Initial delete NC changes Jan 30, 2023
@smittal22 smittal22 force-pushed the saksham_azr_nc_delete branch 2 times, most recently from 78422c1 to 5b18d9d Compare February 2, 2023 17:33
@smittal22 smittal22 force-pushed the saksham_azr_nc_delete branch from 5b18d9d to a88adfd Compare February 3, 2023 17:25
huntergregory and others added 16 commits February 3, 2023 11:26
)

* adaptively modify linux max restore try count to prevent perpetual errors

* remove debug print

* log restore file and send ipsetmanager_linux errors

* send other appropriate errors

* fix handleLineError function

* fix printing restore lines and enhance a log

* fix lints and wrap chainLineNumber errors

* fix one off error for logging the try count

* revert exponential increase to try limit

* update try count to 5 and update UTs

* do not log lines for every restore call until perf is understood
* feat: add cni conflist generator for v4 overlay scenario

* feat: use atomic fs operations

* fix: use same directory as temp dir since /tmp is a tmpfs
There was insufficient coverage over cases involving different
permutations of the "WireserverIP" configuration option. Consequently,
there were instances where reasonable values for this option caused CNS
to fail to start.

This moves the logic for transforming the CNS configuration into
configuration suitable for the NMAgent client into a method off the
CNSConfig. It also permits adding coverage over different scenarios that
are likely to emerge.
fix variable ordering

Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>

Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>

Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
…zure#1668)

* update crd clients to accept existing ctrlcli core clients

Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>

* fix cns to use managed cached clients for reconcilers

Signed-off-by: GitHub <noreply@github.com>

Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
Signed-off-by: GitHub <noreply@github.com>
…or cleanup. (Azure#1606)

* fix: [NPM-WIN] Get only local endpoints to apply ACLs on

* addding a const

* fix lints

* update UTs (TODO: uncomment multi-job test when fixed)

* resolve lint again

* true backwards compatibility

* resolve TODO by uncommenting UT

* fix lint

Co-authored-by: Hunter Gregory <hunterlgregory@gmail.com>
In cases where clients using a WireserverTransport (namely the NMAgent
one) do not wish to provide a body, one must be provided to satisfy
quirks of Wireserver. Initially, an empty Go string was thought to be
sufficient, but this is not. Wireserver expects an empty JSON string:
specifically the 2-byte sequence "".

Furthermore, this logic was only being triggered on cases where a nil
body was presented to the WireserverTransport's logic. Given that there
are now more linters enforcing the use of `http.NoBody`, this is
distinct from `nil` and must be separately checked for (but treated in
the same way).

Tests and inline documentation have been updated to reflect this new
understanding.

* Add Content-Type for outgoing Wireserver POSTs

Because the Body of the request was replaced, Go also has no idea what
the content type should be. It can't read from the reader to perform the
detection, so this just sets it to JSON manually. As mentioned in the
comments, this is a good idea anyway since we're being explicit rather
than relying on heuristic methods to do the right thing.
* Network name as an optional variable
Address s360 items with auto-updates on aks-engine windows clusters
* feat: remove windos 2004 aks-e tests from pipeline

* fix: dependency on cleanup stage

Co-authored-by: Esteban Capillo <estebancams@microsoft.com>
Co-authored-by: estebancams <101819268+estebancams@users.noreply.github.com>
Co-authored-by: Camryn Lee <31013536+camrynl@users.noreply.github.com>
…ld/tools (Azure#1733)

* deps: bump sigs.k8s.io/controller-tools in /build/tools

Bumps [sigs.k8s.io/controller-tools](https://github.com/kubernetes-sigs/controller-tools) from 0.10.0 to 0.11.1.
- [Release notes](https://github.com/kubernetes-sigs/controller-tools/releases)
- [Changelog](https://github.com/kubernetes-sigs/controller-tools/blob/master/RELEASE.md)
- [Commits](kubernetes-sigs/controller-tools@v0.10.0...v0.11.1)

---
updated-dependencies:
- dependency-name: sigs.k8s.io/controller-tools
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* regen crds

Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Evan Baker <rbtr@users.noreply.github.com>
csfmomo and others added 24 commits February 3, 2023 11:26
* Updated aks-engine to unblock the pipeline.

* Use the latest aks-e dependancy.

* Update aks-e dependency to unblock acn pipeline and set retry to 0 to
get quick result.
* Moving the lock from InitializeKeyValueStore() function to restore/save functions to improve cni performance on windows.

* fix: use defer function to unlock statefile.

* fix: fixing the IPAM lock and defer func

* fix: Optimizing cni file lock by moving SetSdnRemoteArpMacAddress() on startup for CRD and MultitenantCRD mode.

* adding store lock on telemetry service start to avoid race condition on windows.
…Azure#1723)

* refactor and move the nmagentConfig code from cns to namgent package.
* update linux conformance binary

* temporarily comment out test profiles until one works

* Revert "temporarily comment out test profiles until one works"

This reverts commit db623d3.

* undo change to git checkout for windows
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>

Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
* Fix incorrect HTTP status from publish NC

CNS was responding with an HTTP status code of "0" from NMAgent.
Successes are supposed to be 200. The C-style var block at the beginning
of publishNetworkContainer was the reason for this. During refactoring,
the location where this status code was set to a successful value of 200
was accidentally removed. Because the var block declared the variable
and silently initialized it to 0, the compiler did not flag this bug as
it otherwise would have. The status code has been removed from this
block and explicitly defined and initialized to a correct value of 200.
Subsequent error handling will change this as necessary.

Also, despite consumers depending on this status, there were no tests to
verify that the status was set correctly. Tests have been added to
reflect this dependency.

* Ensure that NMAgent body is always set

DNC depends on the NMAgent body being set for its vestigial functions of
retrying failed requests. Since failed requests will now be retried
internally (to CNS) by the NMAgent client, this isn't really necessary
anymore. There are versions of DNC out there that depend on this body
though, so it needs to be present in order for NC publishing to actually
work.

* Fix missing NMAgent status for Unpublish

It was discovered that the Unpublish endpoints also omitted the status
codes and bodies expected by clients. This adds those and fixes the
associated tests to guarantee the expected behavior.

* Silence the linter

There were two instances where the linter was flagging dynamic errors,
but this is just in a test. It's perfectly fine to bend the rules there,
since we don't expect to re-use the errors (they really should be
t.Fatal / t.Error anyway, but due to legacy we're returning errors here
instead).
…zure#1717)

* add validation for matchExpression values

* fix lint

* make regex identical to kubectl validation and include more test cases

* remove debugging lines
* ignore certain errors

* remove changes to updatePod tracking (on/off-node)
* tmp commit for onbaording nma v2

* Remove test output file

* Remove unnecessary code when CNS onboard get nc version list without
token

* tmp commit to fix getnc version tests when onboarding nc version api v2
from nmagent.

* Fix the unit test for nmagent v2 api change.

* Fix unit test TestGetNetworkContainerVersionStatus

* Revert back to GetNCVersionF test.

* Roll back to get nc version api v1 for test.

* Continue revert back and store nc version url

* Onboard nmagent get nc version api v2.

* Address pr feedback of returning early and remove comment out code.

* Remove unnecessary ncVersionURLs and NCVersionRequest.

* Remove unnecessary variables.

* Update nmagent get nc version api v2 to v2 url

* Remove comment out code.

* tmp commit for onbaording nma v2

* Remove test output file

* Remove unnecessary code when CNS onboard get nc version list without
token

* tmp commit to fix getnc version tests when onboarding nc version api v2
from nmagent.

* Fix the unit test for nmagent v2 api change.

* Fix unit test TestGetNetworkContainerVersionStatus

* Revert back to GetNCVersionF test.

* Roll back to get nc version api v1 for test.

* Continue revert back and store nc version url

* Onboard nmagent get nc version api v2.

* Address pr feedback of returning early and remove comment out code.

* Remove unnecessary ncVersionURLs and NCVersionRequest.

* Remove unnecessary variables.

* Update nmagent get nc version api v2 to v2 url

* Remove comment out code.

* Update nmagent get nc version list.

* Address feedback and fix golint

* Fix lint issue.

* Fix the remaining 2 lint issues.

* Revert back test error generation to address feedback.
… in /azure-ipam (Azure#1762)

deps: bump github.com/containernetworking/plugins in /azure-ipam

Bumps [github.com/containernetworking/plugins](https://github.com/containernetworking/plugins) from 1.1.1 to 1.2.0.
- [Release notes](https://github.com/containernetworking/plugins/releases)
- [Commits](containernetworking/plugins@v1.1.1...v1.2.0)

---
updated-dependencies:
- dependency-name: github.com/containernetworking/plugins
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>

Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
@timraymond
Copy link
Member

Something is... very strange with the history of this branch. It looks like there was a rebase, since you're the committer on all of those unrelated commits. But I don't think it's based properly, because Github would see that it could be fast-forwarded and show just your commits.

@smittal22
Copy link
Contributor Author

eed to pass two extra params azID (uint) and enableAZR (boolean) as part of body to NMAgent. Since we are not passing body to NMAgent as of now, this PR adds a body to the request just like we have for publish NC.

Whenever NC has enableAZR = true in NC config, we will pass body like: {"azID":<value>,"enableAZR":true} For already existing NC (or if NC has enableAZR = false in NC config), we will pass body like: {"azID": 0,"enableAZR":false}

Issue Fixed:

https://msazure.visualstudio.com/DefaultCollection/One/_workitems/edit/16187805

I'll probably open a new PR once the pipeline issue is resolved. Will take a look at this today.

@smittal22
Copy link
Contributor Author

Closing this PR now. An alternative PR was approved and merged based on new NMAgent proxy.

@smittal22 smittal22 closed this Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.