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

Image value mappings fixes #114

Merged
merged 7 commits into from
Oct 19, 2023

Conversation

jddarby
Copy link
Owner

@jddarby jddarby commented Oct 10, 2023

This PR is to address two issues around the generated VHD parameters for the VNF build path:

  1. Stop hard coding the value of azureDeployLocation. This value should always be configurable.
  2. Add the following optional parameters to the VHD artifact configuration:
    • imageDiskSizeGB
    • imageOsState
    • imageOsType
    • imageHyperVGeneration
    • imageApiVersion

These values are set to defaults by NFM when they are not provided but it is necessary that we give publishers the option to configure them

I have also made the VNF publish and delete test a live test only. This is because it was causing issues for people when running the UTs doe to an underlying issue with the test framework. Please chat to Patryk for more context.

I am planning to merge this into the add-aosm-extension branch and then have a separate PR to add this to the pre-release branch.

@jddarby jddarby requested a review from sunnycarter as a code owner October 10, 2023 14:43
@sunnycarter
Copy link
Collaborator

sunnycarter commented Oct 10, 2023

I've spent a while looking at this and concluded that the AOSM design for this is all way too complicated and prone to error. It's taken me ages of reading old teams threads and looking at the pez codebase to work out what all the various fields do, and I'm still not sure I've got it right in my head.

Having done this, I'm not convinced this is complete. The title of the bug "NF location and the resources deployed by NF has to be same" indicates we need to do more:

  • The NSD deploys an NF using an NF template. That NF template is built at NSD build time, and populated using nf_template.bicep.j2. Line 174 in nsd_generator.py fills in the location parameter for this NF template as the location value from input.json config. That template then deploys an NF resource with location location and networkFunctionDefinitionOfferingLocation (i.e. the location of the NFDV resource) from publisher_offering_location in the input.json config. The first bit of this (location) must be wrong and needs fixing to be supplied by deploymentParameter, the second bit (networkFunctionDefinitionOfferingLocation) is right and ok as is.
  • The azureDeployLocation of the VHD image needs to match that location as well, so that the NF and image end up in the same location
  • The resources deployed by the VM ARM template uploaded as part of the NFDV are also deployed to a location. I assume this must also match the NF location and the azureDeployLocation. IF the CLI user has a VM template with 'location' as a parameter name, then this is all fine. It will be populated by deployParameters.location and all is well with your fix for the image azureDeployLocation. If the VM template has 'jacobsMagicLocation' as the parameter name, then a quick test of your code shows that we currently end up with only one location parameter in the built deploymentParameters.json schema, and that is "jacobsMagicLocation": {
    "type": "string"
    },

What we want to do (I think), is have a single deployment parameter which is used for all 3 of: NF location, image azureDeployLocation and VM template resource locations.

We should warn the user if their VM template doesn't contain a parameter named location and get them to change it so that one deployParameter fills in all values.


And then another thing. Do we currently care about NFDV location (networkFunctionDefinitionOfferingLocation) matching NF location? I cannot remember what AOSMs current limitations are in this area. It's too flipping complicated, that's what I think!!

Finally, it would be wonderful if you could make sure the swagger descriptions for all these fields are descriptive enough that the next time we come to this nobody has to do the same code trawl. I couldn't remember where the GA swagger was, so I haven't checked myself. That clearly isn't part of this story, but if you could check and raise a new story if it isn't clear that would be marvellous.


Going to go and lie down in a dark room now....

@jddarby jddarby merged commit 69e03d1 into add-aosm-extension Oct 19, 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.

2 participants