CASMTRIAGE-1900: Add jinja render to rootfs_provider_passthrough to support iSCSI #265
Conversation
shivaprasad-metimath
force-pushed
the
CASMTRIAGE-1900
branch
15 times, most recently
from
ee616ba to
4103f75
Compare
|
test output from gamora: https://gist.github.com/shivaprasad-metimath/6441895ebdab44ecb19fe7bd27e43593 |
shivaprasad-metimath
changed the title
shivaprasad-metimath
force-pushed
the
CASMTRIAGE-1900
branch
5 times, most recently
from
d2a9eeb to
49583c6
Compare
…rough IM:CASMTRIAGE-1900 Reviewer:Annapoorna
annapoorna-s-alt
force-pushed
the
CASMTRIAGE-1900
branch
from
49583c6 to
df8e1b8
Compare
annapoorna-s-alt
changed the title
There was a problem hiding this comment.
I think it would be good to fix my comments. I opened CRAYSAT-1917 and pushed a branch with a possible fix.
| @@ -242,6 +242,11 @@ def name(self): | |||
| # items that inherit from BaseInputItem. | |||
| return self.data['name'] | |||
|
|
|||
| @property | |||
| def boot_set(self): | |||
There was a problem hiding this comment.
Why was a boot_set property added on the BaseInputItem? This parent class applies to CFS configurations and BOS session templates in the input file, so it's not appropriate to add here. There is also already a boot_sets property on the InputSessionTemplate class where you needed to use it. Can we just use that one?
| boot_sets = self.boot_set | ||
| # Render the rootfs_provider_passthrough using Jinja2 | ||
| rendered_rootfs = self.jinja_env.from_string( | ||
| boot_sets[boot_set_name]['rootfs_provider_passthrough']).render(self.data) |
There was a problem hiding this comment.
Why do we pass in self.data to this? From what I can tell from the Jira, it is not required to allow it to render fields with arbitrary data from the session template. I think there's a way we can just modify and use the jinja_rendered decorator instead.
I think passing in self.data as the context when rendering is going to lead to some weird behavior and possibly hard-to-fix bugs, and then it will be hard to remove the ability to use the data if people start using it that way. I would suggest we not add this unless it's explicitly requested.
| boot_sets = self.boot_set | ||
| # Render the rootfs_provider_passthrough using Jinja2 | ||
| rendered_rootfs = self.jinja_env.from_string( | ||
| boot_sets[boot_set_name]['rootfs_provider_passthrough']).render(self.data) |
There was a problem hiding this comment.
Exceptions also aren't caught here which will lead to tracebacks (e.g. if a variable used in a Jinja expression is not set).
| } | ||
| bos_payload_bootsets[boot_set_name] = { | ||
| 'node_roles_groups': ['Compute'], | ||
| 'rootfs_provider_passthrough': "dvs:api-gw-service-nmn.local:300:hsn0,nmn0:0", |
There was a problem hiding this comment.
It would be good to add a test where the rootfs_provider_passthrough has a variable that needs to be rendered with Jinja2
Reviewer: Annapoorna
Summary and Scope
In order to suport iSCSI in CSM 1.6. jinja rendering of the rootfs_provider_passthrough value is necessary. Hence the functionality to render the value has been added.
For both UAN & Compute support has been added for the input yaml to be handled.
Issues and Related PRs
Testing
List the environments in which these changes were tested.
Tested on:
<Gamora>Test description:
The modified input yaml file consisting of both iSCSI and DVS was considered for testing sat bootprep to create the session templates. It successfully was able to create all the type of templates.
Risks and Mitigations
minimal
Pull Request Checklist