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

Ixia-c operator upgrade #448

Merged
merged 11 commits into from
Nov 9, 2023

Conversation

anjan-keysight
Copy link
Contributor

@alexmasi Keysight manifest and docs update for licensing support in operator.

@anjan-keysight
Copy link
Contributor Author

@alexmasi Pls hold the PR merge, @ashutshkumr will be making some updates to operator library reference, in the same PR

@ashutshkumr
Copy link
Contributor

Pushed following additional changes:

  • Renamed ixia-c images in mock test data too
  • Removed unlicensed deployment (now there's no distinction based on build components - default config-map enables community edition)
  • Renamed ixia tg operator package and versions

What's pending ?

  • Once ixia-c release is out, will be updating the final image versions in config map

@ashutshkumr
Copy link
Contributor

Additional changes:

  • update versions to latest for ixia-c images
  • replace usage of 0.0.1-999 with v0.1 (adding a prefix v will attain the same goal)

@alexmasi @jasdeep-hundal kindly go ahead and review.

We have published images for upcoming releases (but haven't announced yet as we're awaiting PR review).

@alexmasi alexmasi requested a review from marcushines October 20, 2023 20:37
@ashutshkumr
Copy link
Contributor

FYI ixia-c release has been announced. https://github.com/open-traffic-generator/ixia-c/releases/tag/v0.1.0-3

@alexmasi
Copy link
Contributor

I have 3 main concerns with this PR:

  • version v0.0.1 as a placeholder will break existing textprotos using 0.0.0-9999 (featureprofiles and other files we have internally), is this version change necessary? 9999 is just a placeholder that doesn't have anything to do with the real ixia-c versions. Why is this no longer supported?

  • Submitting before google has switched to the new versions (i.e. the equivalents with the old license style) will have vendors testing using different versions with different features / bug fixes, once we receive the first equivalent controller container this can be merged

  • [Non-blocking] The bringup process is now requiring an extra step for non-google users, this should be handled by a step in kne deploy, this can be a PR that adds a field to the deployment config yaml for the ixiatg controller which takes in a ip addr string and if provided creates the necessary secret using the kubeclient:

    type IxiaTGSpec struct {

@ashutshkumr
Copy link
Contributor

I have 3 main concerns with this PR:

  • version v0.0.1 as a placeholder will break existing textprotos using 0.0.0-9999 (featureprofiles and other files we have internally), is this version change necessary? 9999 is just a placeholder that doesn't have anything to do with the real ixia-c versions. Why is this no longer supported?
  • Submitting before google has switched to the new versions (i.e. the equivalents with the old license style) will have vendors testing using different versions with different features / bug fixes, once we receive the first equivalent controller container this can be merged
  • [Non-blocking] The bringup process is now requiring an extra step for non-google users, this should be handled by a step in kne deploy, this can be a PR that adds a field to the deployment config yaml for the ixiatg controller which takes in a ip addr string and if provided creates the necessary secret using the kubeclient:
    type IxiaTGSpec struct {

Regarding first item, we still support 0.0.0-9999, but wanted to understand better the motivation for using this. For KEYSIGHT vendor,

  • If the release is set to x.y.z-abc (valid semver release) in KNE topology, then release information is downloaded from online release file (config map isn't used)
  • If the release is set to vx.y.z-abc (invalid semver release) in KNE topology, then release information won't be found online and hence offline config map with the same release shall be used

Seeing 0.0.0-9999 is just misleading (one might ponder if there is indeed this release in existence). Instead we can use actual release version prefixed with v or use non-release term like offline or custom (so that it's applicable for all releases).

@alexmasi
Copy link
Contributor

alexmasi commented Nov 1, 2023

Regarding first item, we still support 0.0.0-9999, but wanted to understand better the motivation for using this. For KEYSIGHT vendor,

  • If the release is set to x.y.z-abc (valid semver release) in KNE topology, then release information is downloaded from online release file (config map isn't used)
  • If the release is set to vx.y.z-abc (invalid semver release) in KNE topology, then release information won't be found online and hence offline config map with the same release shall be used

Seeing 0.0.0-9999 is just misleading (one might ponder if there is indeed this release in existence). Instead we can use actual release version prefixed with v or use non-release term like offline or custom (so that it's applicable for all releases).

Thanks for explaining how the resolution is done, if in fact the placeholder string does not need to be in a version style then yes I think it makes sense to switch to custom or similar.

The main concern here is backwards compatibility, there exists many kne topology textproto files outside of this repo that use the 0.0.0-9999 pattern. I am concerned that switching this configmap will cause those topologies to break. Are you okay with leaving the admittedly suboptimal 0.0.0-9999 placeholder and I'll open an issue for a longer term switchover to the better placeholder?

@ashutshkumr
Copy link
Contributor

@alexmasi Thanks for feedback. I have,

If there are no other concerns, I think we're ready to merge.

@alexmasi
Copy link
Contributor

alexmasi commented Nov 6, 2023

@alexmasi Thanks for feedback. I have,

If there are no other concerns, I think we're ready to merge.

Thanks, this PR looks good, however there is one remaining issue. We did not yet receive the equivalent release with the old licensing style as agreed upon. I do not want to merge this yet as internal and external users will be testing using different versions (with different functionality). Once we have an exact equivalent of https://github.com/open-traffic-generator/ixia-c/releases/tag/v0.1.0-26 then I will merge this.

@alexmasi
Copy link
Contributor

alexmasi commented Nov 6, 2023

/gcbrun

@alexmasi alexmasi merged commit c5582a3 into openconfig:main Nov 9, 2023
5 checks passed
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.

5 participants