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

Increase pvc size #391

Closed
wants to merge 5 commits into from
Closed

Increase pvc size #391

wants to merge 5 commits into from

Conversation

eguzki
Copy link
Member

@eguzki eguzki commented Jun 18, 2020

Increase PVC resource request:

  • system shared storage: 10Gi
  • Redis system: 10Gi
  • Redis backend: 10Gi
  • System mysql: 10Gi
  • System postgresql: 10Gi

@eguzki eguzki marked this pull request as ready for review June 18, 2020 11:11
@eguzki
Copy link
Member Author

eguzki commented Jun 18, 2020

Warning for the upgrading path:

only dynamically provisioned pvc can be resized and the storageclass that provisions the pvc must support resize

Otherwise upgrade path might fail. Should the operator provide upgrade path? Can be checked if pvc has been provisioned dinamically AND the storageclass suppports resize?

// v1.ResourceStorage must exist, otherwise PVC is regarded as invalid
existingSize := existing.Spec.Resources.Requests[v1.ResourceStorage]

if existingSize.Cmp(desiredSize) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I seem to remember there were some comparison methods that checked unexported fields, and that made detecting differences due to different internal representations of the same value depending on the serialized value. Have we checked this is working as expected in all cases?

That's why helper.CmpResources was created in its day for other kind of resources. I don't remember whether CmpResources takes into account ResourceStorage though

Copy link
Member Author

Choose a reason for hiding this comment

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

CmpResources does not take into account ResourceStorage. Already checked it.

The upgrade logic uses Quantity type comparator, which I expect to be working

func main() {
	desired := resource.MustParse("10Gi")
	existing := resource.MustParse("1Gi")
	fmt.Println("desired == existing", existing.Cmp(desired) == 0)
	existing = resource.MustParse("10Gi")
	fmt.Println("desired == existing", existing.Cmp(desired) == 0)
}
desired == existing false
desired == existing true

@miguelsorianod
Copy link
Contributor

Warning for the upgrading path:

only dynamically provisioned pvc can be resized and the storageclass that provisions the pvc must support resize

Otherwise upgrade path might fail. Should the operator provide upgrade path? Can be checked if pvc has been provisioned dinamically AND the storageclass suppports resize?

That's a concerning issue yes...

I'm also concerned about increasing x10 storage requirements of a 3scale installation.

Let's talk about this a little bit through other channels.

@codeclimate
Copy link

codeclimate bot commented Jun 23, 2020

Code Climate has analyzed commit 1cdb3c2 and detected 6 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1
Style 5

View more on Code Climate.

@eguzki
Copy link
Member Author

eguzki commented Jun 23, 2020

Closing this issue as:

  • the upgrading path may fail and there is no way to prevent it
  • Initial required storage size can be up to 40Gb, which can be too much

A better way to go is:

  • allow "install only" configuration of storage size
  • allow linking PV provisioned by the customer by name (thus no storage size is needed)

@eguzki eguzki closed this Jun 23, 2020
@eguzki eguzki deleted the increase-pvc-size branch August 28, 2020 14:59
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.

None yet

2 participants