Storageclass and snapshotclass refactor

[egafford]

In order to create a minimal test halo for the release of version 4.6, minimal change to the handling of StorageClass and SnapshotClass suggested that we identify the Ceph storage type of each StorageClass or SnapshotClass by slice index for the purpose of looking up management options. This approach will often become somewhat brittle over time. To better support future changes, this patch set creates wrapping structs around StorageClasses and SnapshotClasses that bundle them with their deployment options.

[jarrpa]

Oh I absolutely love this, thank you! I've been meaning to get rid of that indexing...

Overall looks sound. Two naming nits.

[jarrpa]

Make sure to run make ocs-operator-ci before pushing.

[umangachapagain]

While I'm happy to see indices go, I'd be happier if we could get rid of Builder functions returning Pointers to Objects (where/if it makes sense).

I believe this would allow callers to operate on their own copies of objects without any risk of it being tampered by some other caller. I'm not sure if sharing objects built by builder does any good. WDYT?

[egafford]

While I'm happy to see indices go, I'd be happier if we could get rid of Builder functions returning Pointers to Objects (where/if it makes sense).

I believe this would allow callers to operate on their own copies of objects without any risk of it being tampered by some other caller. I'm not sure if sharing objects built by builder does any good. WDYT?

Thanks @umangachapagain. I'll certainly see to the linting changes; much appreciated. As to the question of passing pointers vs. raw structs, I'm not certain I see the risk case that you're describing. Our prior build passed pointers to StorageClasses (and indeed, the StorageClassConfiguration struct now contains such a pointer). I believe that any given caller to (for instance) newCephFilesystemStorageClass will receive a pointer to a new instantiation of the StorageClassConfiguration struct. From there, yes, any one caller can change its own instantiated object through the pointer, but this may be desirable and would also be true if it modified a raw struct and passed it on to another function in its own logical flow.

Am I missing something fundamental in Golang? Do the current functions as written effectively create a flyweight single object and return pointers to that same object to all callers? I am certain that if we created a StorageClassConfiguration as a package variable and returned a pointer to that, side effects between callers could cause problems, but I think the present logic gives a pointer to a new object to each caller (which can then cause its own side effects in its own logical flow, but that might actually be useful).

Or am I missing an error case that I'm failing to see? Looking around the codebase we tend to pass pointers rather than raw structs, but if there's a risk that I'm missing I'm happy to make the changes you suggest.

[jarrpa]

While I'm happy to see indices go, I'd be happier if we could get rid of Builder functions returning Pointers to Objects (where/if it makes sense).
I believe this would allow callers to operate on their own copies of objects without any risk of it being tampered by some other caller. I'm not sure if sharing objects built by builder does any good. WDYT?

Thanks @umangachapagain. I'll certainly see to the linting changes; much appreciated. As to the question of passing pointers vs. raw structs, I'm not certain I see the risk case that you're describing. Our prior build passed pointers to StorageClasses (and indeed, the StorageClassConfiguration struct now contains such a pointer). I believe that any given caller to (for instance) newCephFilesystemStorageClass will receive a pointer to a new instantiation of the StorageClassConfiguration struct. From there, yes, any one caller can change its own instantiated object through the pointer, but this may be desirable and would also be true if it modified a raw struct and passed it on to another function in its own logical flow.

Am I missing something fundamental in Golang? Do the current functions as written effectively create a flyweight single object and return pointers to that same object to all callers? I am certain that if we created a StorageClassConfiguration as a package variable and returned a pointer to that, side effects between callers could cause problems, but I think the present logic gives a pointer to a new object to each caller (which can then cause its own side effects in its own logical flow, but that might actually be useful).

Or am I missing an error case that I'm failing to see? Looking around the codebase we tend to pass pointers rather than raw structs, but if there's a risk that I'm missing I'm happy to make the changes you suggest.

I think you're right that it's a non-issue, at best it would be one of aesthetics. Unless I'm also missing something.

[umangachapagain]

While I'm happy to see indices go, I'd be happier if we could get rid of Builder functions returning Pointers to Objects (where/if it makes sense).
I believe this would allow callers to operate on their own copies of objects without any risk of it being tampered by some other caller. I'm not sure if sharing objects built by builder does any good. WDYT?

Thanks @umangachapagain. I'll certainly see to the linting changes; much appreciated. As to the question of passing pointers vs. raw structs, I'm not certain I see the risk case that you're describing. Our prior build passed pointers to StorageClasses (and indeed, the StorageClassConfiguration struct now contains such a pointer). I believe that any given caller to (for instance) newCephFilesystemStorageClass will receive a pointer to a new instantiation of the StorageClassConfiguration struct. From there, yes, any one caller can change its own instantiated object through the pointer, but this may be desirable and would also be true if it modified a raw struct and passed it on to another function in its own logical flow.
Am I missing something fundamental in Golang? Do the current functions as written effectively create a flyweight single object and return pointers to that same object to all callers? I am certain that if we created a StorageClassConfiguration as a package variable and returned a pointer to that, side effects between callers could cause problems, but I think the present logic gives a pointer to a new object to each caller (which can then cause its own side effects in its own logical flow, but that might actually be useful).
Or am I missing an error case that I'm failing to see? Looking around the codebase we tend to pass pointers rather than raw structs, but if there's a risk that I'm missing I'm happy to make the changes you suggest.

I think you're right that it's a non-issue, at best it would be one of aesthetics. Unless I'm also missing something.

I'm not a Go expert in any way, but my concern was (i) since we pass pointers up to the caller, there's a risk of this memory address being rewritten or cleared so when caller looks at this address it might not get what it is expecting. (Do we check that the object at the address we're trying to read from is actually the object we expected?) Maybe this is not a real thing in which case there's no issue here. (ii) Since we're not trying to share this object and mutate it among the callers, can't we get away with passing a copy to all the callers instead of passing reference to the copies? This seems like a lot of memory allocation at a place where it's not required? (iii) Like you said, each caller gets its own instantiated object and is free to make changes if it wishes to. But, what value does the caller get from having a pointer to this object? If caller or any other method wish to mutate this object, it can just take a reference of this object and not the object at the bottom of the stack?

Maybe I'm completely wrong here hence I made a "Comment' and not "Request Changes". I'm not sure if I'd qualify it as aesthetic change yet. And, yes we've been doing this across the code base but that shouldn't be the reason to continue doing it.

[egafford]

While I'm happy to see indices go, I'd be happier if we could get rid of Builder functions returning Pointers to Objects (where/if it makes sense).
I believe this would allow callers to operate on their own copies of objects without any risk of it being tampered by some other caller. I'm not sure if sharing objects built by builder does any good. WDYT?

Thanks @umangachapagain. I'll certainly see to the linting changes; much appreciated. As to the question of passing pointers vs. raw structs, I'm not certain I see the risk case that you're describing. Our prior build passed pointers to StorageClasses (and indeed, the StorageClassConfiguration struct now contains such a pointer). I believe that any given caller to (for instance) newCephFilesystemStorageClass will receive a pointer to a new instantiation of the StorageClassConfiguration struct. From there, yes, any one caller can change its own instantiated object through the pointer, but this may be desirable and would also be true if it modified a raw struct and passed it on to another function in its own logical flow.
Am I missing something fundamental in Golang? Do the current functions as written effectively create a flyweight single object and return pointers to that same object to all callers? I am certain that if we created a StorageClassConfiguration as a package variable and returned a pointer to that, side effects between callers could cause problems, but I think the present logic gives a pointer to a new object to each caller (which can then cause its own side effects in its own logical flow, but that might actually be useful).
Or am I missing an error case that I'm failing to see? Looking around the codebase we tend to pass pointers rather than raw structs, but if there's a risk that I'm missing I'm happy to make the changes you suggest.

I think you're right that it's a non-issue, at best it would be one of aesthetics. Unless I'm also missing something.

I'm not a Go expert in any way, but my concern was (i) since we pass pointers up to the caller, there's a risk of this memory address being rewritten or cleared so when caller looks at this address it might not get what it is expecting. (Do we check that the object at the address we're trying to read from is actually the object we expected?) Maybe this is not a real thing in which case there's no issue here. (ii) Since we're not trying to share this object and mutate it among the callers, can't we get away with passing a copy to all the callers instead of passing reference to the copies? This seems like a lot of memory allocation at a place where it's not required? (iii) Like you said, each caller gets its own instantiated object and is free to make changes if it wishes to. But, what value does the caller get from having a pointer to this object? If caller or any other method wish to mutate this object, it can just take a reference of this object and not the object at the bottom of the stack?

So my understanding is that of one of the fundamental design goals of Golang was to use pointers (and thus make pass-by-reference and pass-by-value explicit), but to disallow pointer math entirely. As pointer math is hard to debug, easy to get wrong, and more or less never strictly needed, this idea seems pretty great. So as I understand it, Golang protects us from the danger of rewriting the address.

However, you have two points here. The second is more about efficiency and the Golang idiom. In the Golang runtime, is it more efficient to pass around a (smaller) memory address on the stack but need to dereference to get to the object, or is it more efficient to pass the (larger) raw struct itself? And perhaps more importantly, what is idiomatic in Golang and will make it easier for future developers to understand naturally? This question I'm truly not sure about, as I've worked in Golang less than others on the team, and it's a good one to ask.

@jarrpa, any thoughts on idiomatic Golang and preference for passing pointers or raw structs?

Maybe I'm completely wrong here hence I made a "Comment' and not "Request Changes". I'm not sure if I'd qualify it as aesthetic change yet. And, yes we've been doing this across the code base but that shouldn't be the reason to continue doing it.

[jarrpa]

However, you have two points here. The second is more about efficiency and the Golang idiom. In the Golang runtime, is it more efficient to pass around a (smaller) memory address on the stack but need to dereference to get to the object, or is it more efficient to pass the (larger) raw struct itself? And perhaps more importantly, what is idiomatic in Golang and will make it easier for future developers to understand naturally? This question I'm truly not sure about, as I've worked in Golang less than others on the team, and it's a good one to ask.

@jarrpa, any thoughts on idiomatic Golang and preference for passing pointers or raw structs?

I did a bit of looking around and I found something that may mean Umanga's suggestion is more idiomatic:

Passing by value often is cheaper

Even though Go looks a bit like C, its compiler works differently. And C analogy does not always work with Go. Passing by value in Go may be significantly cheaper than passing by pointer. This happens because Go uses escape analysis to determine if variable can be safely allocated on function’s stack frame, which could be much cheaper then allocating variable on the heap. Passing by value simplifies escape analysis in Go and gives variable a better chance to be allocated on the stack.

https://goinbigdata.com/golang-pass-by-pointer-vs-pass-by-value/

So, I'll say we should go ahead with his suggestion.

[egafford]

However, you have two points here. The second is more about efficiency and the Golang idiom. In the Golang runtime, is it more efficient to pass around a (smaller) memory address on the stack but need to dereference to get to the object, or is it more efficient to pass the (larger) raw struct itself? And perhaps more importantly, what is idiomatic in Golang and will make it easier for future developers to understand naturally? This question I'm truly not sure about, as I've worked in Golang less than others on the team, and it's a good one to ask.
@jarrpa, any thoughts on idiomatic Golang and preference for passing pointers or raw structs?

I did a bit of looking around and I found something that may mean Umanga's suggestion is more idiomatic:

Passing by value often is cheaper
Even though Go looks a bit like C, its compiler works differently. And C analogy does not always work with Go. Passing by value in Go may be significantly cheaper than passing by pointer. This happens because Go uses escape analysis to determine if variable can be safely allocated on function’s stack frame, which could be much cheaper then allocating variable on the heap. Passing by value simplifies escape analysis in Go and gives variable a better chance to be allocated on the stack.

https://goinbigdata.com/golang-pass-by-pointer-vs-pass-by-value/

So, I'll say we should go ahead with his suggestion.

Cool; will do. Thanks y'all.

[umangachapagain]

/retest

[umangachapagain]

I'm not a Go expert in any way, but my concern was (i) since we pass pointers up to the caller, there's a risk of this memory address being rewritten or cleared so when caller looks at this address it might not get what it is expecting. (Do we check that the object at the address we're trying to read from is actually the object we expected?) Maybe this is not a real thing in which case there's no issue here. (ii) Since we're not trying to share this object and mutate it among the callers, can't we get away with passing a copy to all the callers instead of passing reference to the copies? This seems like a lot of memory allocation at a place where it's not required? (iii) Like you said, each caller gets its own instantiated object and is free to make changes if it wishes to. But, what value does the caller get from having a pointer to this object? If caller or any other method wish to mutate this object, it can just take a reference of this object and not the object at the bottom of the stack?

So my understanding is that of one of the fundamental design goals of Golang was to use pointers (and thus make pass-by-reference and pass-by-value explicit), but to disallow pointer math entirely. As pointer math is hard to debug, easy to get wrong, and more or less never strictly needed, this idea seems pretty great. So as I understand it, Golang protects us from the danger of rewriting the address.

Yes, you're correct. Golang protects us from this issue. There is no data corruption or integrity issue here like I thought. But, this came at the cost of heap allocations (Go's escape analysis did this) which also means more work for Garbage Collector?

This became clear when I ran a basic benchmarking test. While returning pointers newStorageClassConfigurations() was doing 24 allocations/operation and cycles were spent to put object into heap, retrieve it back and GC etc. Without pointers we were at 22 allocations/operation.

#925 (comment) validates the finding.

[jarrpa]

The ocs-operator-ci failure is a flake. But will wait for the rebase to trigger it again.

[egafford]

The ocs-operator-ci failure is a flake. But will wait for the rebase to trigger it again.

Groovy; seemed that way from my local results but I wanted to investigate a bit more before assuming anything to not be my fault. :)

I'll go ahead and rebase then. Thanks.

[umangachapagain]

/retest

[openshift-merge-robot]

@egafford: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/red-hat-storage-ocs-ci-e2e-aws	e2fddf7	link	/test red-hat-storage-ocs-ci-e2e-aws

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/test-infra repository. I understand the commands that are listed here.

[obnoxxx]

Great start, thanks.

Looking at these pieces of code in the last couple of days, I started to have a slightly different idea for a refactoring.

Currently it is like this:

ensureStorageClasses() {
  scs = newStorageClasses()

  createStorageClasses(scs)
}

One of the problem is that we do this as a list in the first place.
Secondly, a problem is that some of the filtering happens in newStorageClasses and some in createStorageClasses.

So I was thinking of doing it roughly like this:

func ensureStorageClasses() {
  ensureRbdStorageClass()
  ensureCephFsStorageClass()
  ensureOBCStorageClas()
}

func ensureRbdStorageClass() {
  if avoidRbdStorageClass() {
    return
  }
  sc := newRbdStorageClass()
  createStorageClass(sc)
}

func ensureCephfsStorageClass() {
  if avoidCephFsStorageClass() {
     return
  }
  sc := newCepFsStorageClass()
  createStorageClass(sc)
}

func ensureObcStorageClass() {
  if avoidObcStorageClass() {
    return
  }
  sc := newObcStorageClass()
  createStorageClass(sc)
}   

The newFooStorageClass() would be exactly from your first commit.
And all the filtering goes into the avoidFooSC functions.

Just a rough sketch.

What do you think?

FWIW, yesterday, I have started a patch to first consolidate the filtering in createStorageClasses first.

[obnoxxx]

/hold

holding b/c of discussion of approach

[obnoxxx]

FYI: #954 is the WIP PR where I consolidated the filtering for storage classes.

[jarrpa]

Responding to @obnoxxx 's proposed refactor:

There's a consequence that this would involve some refactoring to the external resources code as well, since we would now need to do individual create wrapper function calls for each StorageClass as we encounter them rather than creating them all after all information is gathered.

This would also necessitate breaking up the StorageClassConfiguration struct. The reason being that the struct contains the information as to whether the SC would be created or not. Admittedly, this is just a convenience to not duplicate the code in createStorageClasses(). From that standpoint I prefer leaving things as they are, to avoid code duplication. The only readon for the split in filtering is because the OBC SC needs to also check if initData.Spec.ExternalStorage.Enable || err == nil && !avoidObjectStore(platform) , but otherwise the filtering checks are the same.

[obnoxxx]

Responding to @obnoxxx 's proposed refactor:

There's a consequence that this would involve some refactoring to the external resources code as well, since we would now need to do individual create wrapper function calls for each StorageClass as we encounter them rather than creating them all after all information is gathered.

Right, there was something about that caller... But it might be fine to refactor that a bit too, since this is a little horrible anyway... 😉

This would also necessitate breaking up the StorageClusterConfiguration struct.

Well, in my approach, the StorageClassConfiguration does not necessarily exist. We might not need it.
But I do not understand why we would need to break it up yet, since it is very generic.

The reason being that the struct contains the information as to whether the SC would be created or not.

Right, but that information is rather generic. Let me thing a bit more about it to understand better.

Admittedly, this is just a convenience to not duplicate the code in createStorageClasses(). From that standpoint I prefer leaving things as they are, to avoid code duplication.

I didn't intend to introduce a lot of code duplication. But get rid of lists and case/if-else statements.

The only readon for the split in filtering is because the OBC SC needs to also check if initData.Spec.ExternalStorage.Enable || err == nil && !avoidObjectStore(platform) , but otherwise the filtering checks are the same.

This is not needed, afaict, and I started to get rid of it in #945.

Let me think a bit more about it...

[egafford]

So what I'm seeing here on review is that we do have a different set of filter criteria in the managed vs external ObjectStore. We could try to implement a formal strategy or behavior pattern around this filter. (The caller of createStorageClasses could pass a filter set defined by an interface.) That would likely be more complex than the current implementation, though, and I'm not certain there's a real improvement in maintainability by doing so.

I'm not in favor of pushing logic specific to the external ObjectStore case into storageclasses.go; to my mind that would be a pretty clear violation of encapsulation that would make the code harder to maintain.

Responding to @obnoxxx 's proposed refactor:
There's a consequence that this would involve some refactoring to the external resources code as well, since we would now need to do individual create wrapper function calls for each StorageClass as we encounter them rather than creating them all after all information is gathered.

Right, there was something about that caller... But it might be fine to refactor that a bit too, since this is a little horrible anyway... wink

This would also necessitate breaking up the StorageClusterConfiguration struct.

Well, in my approach, the StorageClassConfiguration does not necessarily exist. We might not need it.
But I do not understand why we would need to break it up yet, since it is very generic.

The reason being that the struct contains the information as to whether the SC would be created or not.

Right, but that information is rather generic. Let me thing a bit more about it to understand better.

Admittedly, this is just a convenience to not duplicate the code in createStorageClasses(). From that standpoint I prefer leaving things as they are, to avoid code duplication.

I didn't intend to introduce a lot of code duplication. But get rid of lists and case/if-else statements.

The only readon for the split in filtering is because the OBC SC needs to also check if initData.Spec.ExternalStorage.Enable || err == nil && !avoidObjectStore(platform) , but otherwise the filtering checks are the same.

This is not needed, afaict, and I started to get rid of it in #945.

Let me think a bit more about it...

[obnoxxx]

So what I'm seeing here on review is that we do have a different set of filter criteria in the managed vs external ObjectStore. We could try to implement a formal strategy or behavior pattern around this filter. (The caller of createStorageClasses could pass a filter set defined by an interface.) That would likely be more complex than the current implementation, though, and I'm not certain there's a real improvement in maintainability by doing so.

Right... I think it's not worth it.

I'm not in favor of pushing logic specific to the external ObjectStore case into storageclasses.go; to my mind that would be a pretty clear violation of encapsulation that would make the code harder to maintain.

Currently, there is some external specific logic in storageclasses.go, but not needed (any more).

I think the current patch here is a clear improvement as is and is also preparing for possible further changes.

[obnoxxx]

/hold cancel

[egafford]

For what it's worth, @obnoxxx, on another review, I think I see what you're saying, which is to just call directly into the creation functions for each storage type directly from the clients of storageclasses.go (whether local or external).

I'm also seeing that we create other k8s resources (ConfigMaps and Secrets, notably) inline in the case statement of external_resources, so it wouldn't be out of keeping with our current codebase. (This was the piece I was missing earlier.)

Sorry for not fully getting your proposal earlier. I'll poke at this tomorrow and see whether it's straightforward to implement. If it's not, I think you're correct that this PR is a step in the right direction as-is.

[egafford]

@obnoxxx, after some examination playing around with external_resources.go, in order to make the individual functions for {avoid, create} * {fs, blockpool, obc}, we would have to duplicate a fair bit of code, especially around error handling and negative cases. It may be that a deeper refactoring of external_resources.go itself would allow us to do this sensibly. Still, while I do very much like your proposed approach from a code cleanliness point of view, I think this patch may well be the right approach, for now at least.

[jarrpa]

The code looks good, but I think there's a small cleanup required between commits.

[jarrpa]

I believe this block should be in the previous commit.

[egafford]

@jarrpa, while the OBC index was removed in the previous commit, createSnapshotClasses in volumsnapshotterclasses.go still relies on these two index values in its switching block.

[jarrpa]

Ah! I see. Thanks!

[jarrpa]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jarrpa, umangachapagain

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:

• OWNERS [jarrpa,umangachapagain]

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

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.