This repository has been archived by the owner on Mar 31, 2023. It is now read-only.
[Node Mgr] Node Manager Interface for NCM Registration #598
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
IMPORTANT NOTE: This is a breaking change in the following way: create/update node will fail if NodeInfo doesn't contain a valid NCM uri. This means, 1) An instance of NCM must be running, 2) The NCM should be registered with NMM first. NCM Registration should contain the valid http end-point (like, http://localhost:9014) and an ID. NodeInfo should contain this NCM ID. delete node will work only if an NCM is up and running at http://localhost:9014. Currently this is an issue, it is defered to next commit since it requires solving a rather tricky problem but it will be fixed as soon as possible. The same conditions apply to bulk/file upload methods to create, update and delete nodes through NMM. Fix known issues in previous commit - Node creation returns error if the ncm_id has not been registered. - NMM query by ncm_id is working. - NMM is using NCM URI to talk to NCM, except for DELETE (pending issue) - No Unit tests yet, only swagger UI has been used to validate the code. Unit tests don't seem to test the real API, the integration test script for testing NMM to NCM flow will be extended.
Codecov Report
@@ Coverage Diff @@
## master #598 +/- ##
============================================
- Coverage 32.64% 32.64% -0.01%
+ Complexity 1239 1238 -1
============================================
Files 521 521
Lines 13079 13070 -9
Branches 1603 1598 -5
============================================
- Hits 4270 4267 -3
+ Misses 8230 8223 -7
- Partials 579 580 +1
Continue to review full report at Codecov.
|
Good to make a breaking change explicit. Do we support node registration with an empty or null NCM id? No. It will fail. How about introducing a concept of "default NCM" to support node lifetime management |
cj-chung
reviewed
Apr 15, 2021
lib/src/main/java/com/futurewei/alcor/common/db/ignite/MockIgniteServer.java
Outdated
Show resolved
Hide resolved
lib/src/main/java/com/futurewei/alcor/common/rbac/StrictRbacManager.java
Outdated
Show resolved
Hide resolved
cj-chung
reviewed
Apr 15, 2021
...manager/src/main/java/com/futurewei/alcor/nodemanager/service/implement/NodeServiceImpl.java
Show resolved
Hide resolved
cj-chung
reviewed
Apr 15, 2021
web/src/main/java/com/futurewei/alcor/web/restclient/NetworkConfigManagerRestClient.java
Outdated
Show resolved
Hide resolved
IMPORTANT NOTE: - This is yet another breaking change. This commit introduces named versions of NodeInfo cache in NMM, DPM and NCM. Previously, the three were physically the same cache. If any code/test assumes the existence of certain node in these caches before this change, they will not find those entries since this change. creates new caches. - This commit also introduces an implicit assumption that bulk adding of nodes can only be done to one NCM at a time. Even if each nodeinfo has a distinct NCM id, NMM will pick the NCM Id from the first nodeinfo and send the whole batch to that NCM. - Node creation via file upload was meant to be an admin task and was not designed to dispatch those nodes to DPM and NCM. This commit does not change this old behavior. Regular change notes: - Node creation will continue to work if nodeinfo has no ncmid and no NCM is running, or NCM is running but has no NCM is registered with NMM. - Update integration test script (scripts/test/nmm-ncm-test.sh) with NCM id/uri. UPDATE is also validated. - Fix update/delete getting called through wrong methods in DPM/NCM rest client code. Using rest template exchange method with correct HTTP headers is required in those sites.
Codecov Report
@@ Coverage Diff @@
## master #598 +/- ##
============================================
+ Coverage 32.67% 32.71% +0.03%
- Complexity 1242 1247 +5
============================================
Files 522 522
Lines 13096 13093 -3
Branches 1610 1606 -4
============================================
+ Hits 4279 4283 +4
+ Misses 8236 8232 -4
+ Partials 581 578 -3
Continue to review full report at Codecov.
|
xieus
approved these changes
Apr 21, 2021
...es/node_manager/src/main/java/com/futurewei/alcor/nodemanager/controller/NodeController.java
Show resolved
Hide resolved
web/src/main/java/com/futurewei/alcor/web/restclient/NetworkConfigManagerRestClient.java
Outdated
Show resolved
Hide resolved
|
@pkommoju Please create a tracking issue for every outstanding item. |
__IMPORTANT NOTE__ * This is yet another breaking change. This commit introduces named versions of NodeInfo cache in NMM, DPM and NCM. Previously, the three were physically the same cache. If any code/test assumes the existence of certain node in these caches before this change, they will not find those entries since this change creates new caches. * This commit also introduces an implicit assumption that bulk adding of nodes can only be done to one NCM at a time. Even if each nodeinfo has a distinct NCM id, NMM will pick the NCM Id from the first nodeinfo and send the whole batch to that NCM (issue futurewei-cloud#601 is tracking this) * Node creation via file upload was meant to be an admin task and was not designed to dispatch those nodes to DPM and NCM. This commit does not change this old behavior (issue futurewei-cloud#600 is tracking this) Regular change notes: * Node creation will continue to work if nodeinfo has no ncmid to be backward compatible. Delete, update file upload, bulk add all are 100% backward compatible if ndmid is missing. * If ncmid is present, it must resolve to a registered NCM or an error will be raised. * Update integration test script (scripts/test/nmm-ncm-test.sh) with NCM id/uri. UPDATE is also validated. Added node creation with no NCMID. * Fix update/delete getting called through wrong methods in DPM/NCM rest client code. Using rest template exchange method with correct HTTP headers is required in those sites.
xieus
changed the title
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
IMPORTANT NOTE
versions of NodeInfo cache in NMM, DPM and NCM. Previously, the three
were physically the same cache.
If any code/test assumes the existence of certain node in these caches
before this change, they will not find those entries since this change.
creates new caches.
This commit also introduces an implicit assumption that bulk adding of
nodes can only be done to one NCM at a time. Even if each nodeinfo has a
distinct NCM id, NMM will pick the NCM Id from the first nodeinfo and
send the whole batch to that NCM (issue Bulk node creation forces all nodes to be created in the NCM found in the first nodeinfo. #601 is tracking this)
Node creation via file upload was meant to be an admin task and was
not designed to dispatch those nodes to DPM and NCM. This commit
does not change this old behavior (issue Node Manager should propogate nodes created through file upload to DPM and NCM. #600 is tracking this)
Regular change notes:
Node creation will continue to work if nodeinfo has no ncmid to be backward
compatible. Delete, update file upload, bulk add all are 100% backward
compatible if ndmid is missing.
If ncmid is present, it must resolve to a registered NCM or an error will
be raised.
Update integration test script (scripts/test/nmm-ncm-test.sh) with NCM
id/uri. UPDATE is also validated. Added node creation with no NCMID.
Fix update/delete getting called through wrong methods in DPM/NCM rest
client code. Using rest template exchange method with correct HTTP
headers is required in those sites.