Skip to content

Conversation

@averdagu
Copy link
Contributor

@averdagu averdagu commented Jan 8, 2025

Doing data-plane adoption while testing scenario A (different subnets between wallaby and next-gen) will result in connectivity problem since pods will try to reach old CP using default route (since both CP are on different subnets) and won't be able to reach it.

Adding this route in internalapi to support data-plane adoption scenario A gates.

@openshift-ci openshift-ci bot requested review from fao89 and viroel January 8, 2025 13:50
@averdagu averdagu requested review from karelyatin and removed request for fao89 and viroel January 8, 2025 13:50
"range_end": "${INTERNALAPI_PREFIX}.70"
EOF_CAT
# In the data-plane adoption scenario A (where different IP subnet ranges
# between next-gen and wallaby are used) the net-attach-def needs additional routes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should allow generic routes to be specified set with parameters, by default non will be set and by exporting env vars those could be set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, implementing it now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wanted to have this generic for all the networks not just internalapi
Also can't we reuse other existing vars *_HOST_ROUTES instead of creating similar var?

@averdagu averdagu force-pushed the feat/scenario-a-routes branch 2 times, most recently from 21a840e to bbf87f3 Compare January 14, 2025 13:18
}
]
EOF_CAT
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about storage and storage_mgmt networks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per discussion a while back on nova channel, it is not possible to modify this two networks during dataplane adoption

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The route in net-attach is generic, not specific to adoption, just needed for adoption case. so having it generic for all involved network is good.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI fails as malformed json is generated, i think that could be handled by always adding the empty routes section, and when overrides are provided add to the routes section ?
Also i think let's be generic, i.e the patch should be Allow passing routes to all the network attachments including storage/storagemgmt etc. Similar to what being done for nncp. And remove the comments specific to adoption scenarios.
Dataplane scenario is just an use case of this support.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found out why json was malformated, handled already.
Also I added the ability to pass routes to all networks (storage/storagemgmt) as on the nncp section.
Still missing dataplane and designate. don't know if I should also add it

Doing data-plane adoption while testing scenario A (different
subnets between wallaby and next-gen) will result in connectivity
problem since pods will try to reach old CP using default route
(since both CP are on different subnets) and won't be able to reach it.

Adding this route in internalapi to support data-plane
adoption scenario A gates.

Currently only one route can be added, if needed logic can be improved
to include more than one route.

Ref: OSPRH-5602
@averdagu averdagu force-pushed the feat/scenario-a-routes branch from bbf87f3 to 5e24ee3 Compare February 11, 2025 11:18
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 11, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: averdagu
Once this PR has been reviewed and has the lgtm label, please ask for approval from karelyatin. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@softwarefactory-project-zuul
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/38f31d563b3c4000bcf84c51056ec7b2

✔️ openstack-k8s-operators-content-provider SUCCESS in 46m 08s
install-yamls-crc-podified-edpm-baremetal FAILURE in 31m 44s
podified-multinode-edpm-deployment-crc FAILURE in 27m 39s

@openshift-merge-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 11, 2025

@averdagu: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/precommit-check 5e24ee3 link true /test precommit-check

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants