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

fix: Fix concurrency issues with kube config against same cluster #328

Closed
wants to merge 1 commit into from

Conversation

vburckhardt
Copy link

@ocofaigh
Copy link

@hkantare @kavya498 Can you help get this merged please?

@hkantare
Copy link
Collaborator

We are reviewing PR and will be part of next week terraform provider release

@hkantare
Copy link
Collaborator

@ocofaigh we reviewed the PR its creating a new folder for each action...Lets say if user run terraform plan or apply multiple times then it will have its own directory created ..This way the folder structures gets added in each run and doesn't get cleaned up
Screenshot 2021-10-26 at 4 06 46 PM

@hkantare
Copy link
Collaborator

Instead of generating a new folder I think the better approcah is to add mutex (terraform mutex) on datasource something similar where we have lock on cluster id so that the provider executes the datasources serially
https://github.com/IBM-Cloud/terraform-provider-ibm/blob/21d604abd854c46e3c61626fbe72f1c22962f07d/ibm/resource_ibm_is_instance_group_manager_policy.go#L132

@ocofaigh
Copy link

@hkantare This is intended behaviour, otherwise concurrent data lookups on the same cluster will fail. What seems to happen with the current behaviour is:

  1. The directory gets created in $HOME, and a config.zip file gets added to it.
  2. A config.yml is extracted from the zip
  3. The config.zip is then deleted.

When this process happens concurrently, you get the error: Error downloading the cluster config [c3en2rss08p3fuce3dk0]: open /Users/conall/3b823a3d3a844d109ac91ffba785f0b3eef39cb03dd513926c4e396a81f755e7_c3en2rss08p3fuce3dk0_k8sconfig/config.zip: no such file or directory

I guess another approach could be to add a lock on that process so it cannot be executed concurrently?

@hkantare
Copy link
Collaborator

I think we will go with another approcah to add the mutex from Terraform level
https://github.com/IBM-Cloud/terraform-provider-ibm/blob/b6b22f98829ac15c46b6c96166d52a5af4313067/ibm/internal/mutexkv/mutexkv.go

unc dataSourceIBMContainerClusterConfigRead(d *schema.ResourceData, meta interface{}) error {
	csClient, err := meta.(ClientSession).VpcContainerAPI()
	if err != nil {
		return err
	}
	csAPI := csClient.Clusters()
	name := d.Get("cluster_name_id").(string)
	download := d.Get("download").(bool)
	admin := d.Get("admin").(bool)
	configDir := d.Get("config_dir").(string)
	network := d.Get("network").(bool)
	clusterId := "Cluster_Config_" + name
	ibmMutexKV.Lock(clusterId)
	defer ibmMutexKV.Unlock(clusterId)
	 -------

@hkantare
Copy link
Collaborator

we are looking in above aproach

@ocofaigh
Copy link

ocofaigh commented Nov 1, 2021

No need for this PR anymore - it can be closed since the fix is in IBM-Cloud/terraform-provider-ibm#3264 which is in v1.35.0

@hkantare
Copy link
Collaborator

hkantare commented Nov 2, 2021

closing the PR

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.

3 participants