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

Use IP addresses associated with network attachments #272

Merged

Conversation

ASBishop
Copy link
Contributor

@ASBishop ASBishop commented Sep 29, 2023

Configure the cinder-volume service's target_ip_address and
target_secondary_ip_addresses using addresses in the network attachment
status. This is necessary in order for EDPM compute nodes to be able to
make volume attachment requests (via iSCSI or NVMe/TCP) over the storage
network.

Jira: OSP-28445

Copy link
Contributor

@Akrog Akrog left a comment

Choose a reason for hiding this comment

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

I have yet to test it, though I'm sure it will work fine, but still I think we should support multiple secondary IP addresses.

controllers/cindervolume_controller.go Outdated Show resolved Hide resolved
controllers/cindervolume_controller.go Outdated Show resolved Hide resolved
@ASBishop ASBishop force-pushed the storage-network-addrs branch 2 times, most recently from 2f62aee to 4660970 Compare October 3, 2023 16:43
// but it's safe (and easier) to always configure them regardless of the
// backend type. The configuration also relies on the fact that the LVM
// backend must have only one replica.
if len(networkAttachmentAddrs) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you consider doing this conditionally only when using LVM backends?

The idea would be to change the extendCustomServiceConfig that is being called in L556 to return 2 values, the customServiceConfig and a boolean for usesLVM. We may need to change the method name.

The extendCustomServiceConfig shouldn't need a lot of code changes, basically:

  • Not break on token == "enabled_backends"
  • Create a new variable hasLVM := false and another foundDrivers := 0
  • Get the value as well as the token we are getting on SplitN
  • Add a new } else if token == "volume_driver" && value == "cinder.volume.drivers.lvm.LVMVolumeDriver" { where we set hasLVM to true and increase the foundDrivers counter
  • At the end the actual boolean that determines whether there's LVM or not would be hasLVM && len(backendNames) == foundDrivers` because we have to account that it's the default driver and its value could be implicit.

The reason for this is to avoid restarting the container when not using LVM, which are all the production environments.

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, done!

@ASBishop ASBishop force-pushed the storage-network-addrs branch 2 times, most recently from 1552551 to dcc9ab6 Compare October 4, 2023 19:51
@ASBishop
Copy link
Contributor Author

ASBishop commented Oct 4, 2023

/retest

unknown failure deploying openstack-operator

Configure the cinder-volume service's target_ip_address and
target_secondary_ip_addresses using addresses in the network attachment
status. This is necessary in order for EDPM compute nodes to be able to
make volume attachment requests (via iSCSI or NVMe/TCP) over the storage
network.

Jira: OSP-28445
@ASBishop ASBishop force-pushed the storage-network-addrs branch from dcc9ab6 to 7cd5920 Compare October 5, 2023 20:39
Copy link
Contributor

@Akrog Akrog left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 6, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 6, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Akrog, ASBishop

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

The pull request process is described 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

@openshift-ci openshift-ci bot merged commit ce89e0f into openstack-k8s-operators:main Oct 6, 2023
1 check passed
@ASBishop ASBishop deleted the storage-network-addrs branch October 6, 2023 15:40
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.

2 participants