Fix "Invalid environment variabled" error for multi-container rockons with "environment" section

[anatox]

Used to fix #1588.
env_map contains flat list of environment varialbes, so while looping through containers and checking for environment variables from that list there should not be any exception. Just ignore it and move on to the next container in loop.
Even if no such containers exists (just imagine), that would not be any problem, at least nothing worth an exception, but that's another story.

[schakrava]

Can one of the admins verify this patch?

[schakrava]

Can one of the admins verify this patch?

[schakrava]

Can one of the admins verify this patch?

[schakrava]

Can one of the admins verify this patch?

[MFlyer]

@schakrava : probably this will be my first code approach to Rock-ons, will check it ;)

Mirko
P.S.: your messages get doubled?!?

[MFlyer]

To @anatox @schakrava and @phillxnet
Premise : I've never had code on rock-ons, but this time probably this help!

My opinion : back to @anatox opening post, we skip any check for wrong environment vars so we don't raise any error, but so doing we completely change business logic aka "update if exists, don't care if var doesn't exist"
@schakrava and @phillxnet : don't know DContainverEnv - Rockons relationship, can this deleted check break Rockons expected behaviours?

[anatox]

An error could be raised if no variables are found for any container of the whole for co in containers: loop. It would be close to original idea. However I believe that in this case raising an error is a bit reduntant or at least the text should be changed. It looks (if ever raised) like the error is in json but it is in rockstor database actually. It's not fatal anyway and doesn't prevent from installing the rockon so maybe just log a warning?

So there are options and I could modify this pr in if needed.

[MFlyer]

It looks (if ever raised) like the error is in json but it is in rockstor database actually

You got it @anatox : my knowledge about Rockstor db table for Rock-ons vs json structure doesn't let me understand if we can skip it or not / if we miss something on db side (With my current knowledge I'm more for "wrong/missing" db datas). Once again I'm sure @phillxnet & @schakrava probably can help on this

[MFlyer]

Finally had my "welcome to Rockons world", it was fun! and I say "No, I don't agree on this"

Let me explain @anatox:

• Rockstor Rockons db entries filled from json files, found it reading/simulating code from rockon.py under storageadmin/views
• On every Rockons "Update" button click we perfor a POST request to rockon.py view to sync db env vars to real json files vars
• Here we start our post request, getting available rockons (rockons = self._get_available()) then updating db entries with self._create_update_meta(r, rockons[r]) for every container/docker image
• _create_update_meta then updates env vars too here and finally _update_env has its get_or_create over DContainerEnv table

After this long talk, my opinion:

• A: we can have some errors on rockon.py when creating db entries
• B: we can have some wrong json scrutures
• A+B <- worst case

Current PR is a solution, but so doing we don't check & fix the real issue: it's like hiding broken floor tiles with a carpet, we don't see them anymore, but they're still there 😉

Please see this as an invitation to contribute 😃

[MFlyer]

Add @anatox : can you provide a json file generating this issue? So I can help testing :)

[anatox]

@MFlyer, my pr is not hiding anything. The error removed is actually a mistake, if we want this error to be raised anway, it should raised somewhere else. You have expaned how rockons are stored in DContainerEnv table of rockstor database. I know that, even examined database with pgadmin just to find out there is no error there an nothing is missed. But the problem I'm trying to solve is when rockon is installed in rockon_id.py.

Imagine we have some rockon Rock1. It consists of two containers: ContainerA and ContainerB. ContainerA has ENVA1 and ENVA2 variables and ContainerB has ENVB1. And that's what happens (for now):

• env_map = request.data.get('environment', {}) -here we read all variables with user-assigned values in a plain list. In our example it would be [['ENVA1', 'valuea1'], ['ENVA2', 'valuea2'], ['ENVB1', 'valueb1']]
• for co in containers: - here we run through all the containers in rockon
• • for e in env_map.keys() - here we run though all environment variables in env_map we have filled with values before
• • • if (not DContainerEnv.objects.filter(container=co, key=e).exists()): - checking if this container - variable relation is stored in database. ContainerA: ENVA1 is OK. ENVA2 is OK and ENVB1 is not OK and an error will raise. And because env_map contains everything this code will allways check all variables from all the containers even when it should not. Thus no multicontainer environment will ever work (with defined environment variables)

Here is real json. It's not ready yet but ok to test for this error :) The install will fail when checking nextcloud container for MYSQL_ROOT_PASSWORD from nextcloud-mariadb container

[MFlyer]

my pr is not hiding anything

While writing my last answer i thought about possible "misunderstandings" and I really hope you did not see it like a judgement :)
My suggestion: I think you can master this issue and your last post confirms this idea 🎉 - with "master this issue" I mean you can think about db redesign, code refactoring, etc etc...and I really hope you'll do it!!! We really appreciate new contributors 😉

[anatox]

Thank you, @MFlyer, I didn't see it like a judgement. I just wanted to show that removing an error check is not a 'hack' or something but a real solution :) I will try to "master this issue" and moreover I've already found another bug in the same function so I'll better change original pr a little.

[schakrava]

Appreciate all your input on this @anatox and @MFlyer . I'll take a look soon. My repeated messages up there are automated once after a pr check failure. Sorry about that.

[anatox]

Sorry for long delay. Restored the 'invalid variable' error but now all variables are written and then read back separately. So everything works without breaking global code style.
To simplify testing I created this rockon-test.json

[FroggyFlox]

I was suffering from the same error while trying to write my own custom Rock-on, and the two commits in this fix resolved the issue. I am now able to use environment variables in linked containers within the same Rock-on.

[phillxnet]

@FroggyFlox is this one now sorted by other means? It's now super old and much has changed in the interim time. It would also require a re-base and it's now been so long that may not be a reasonable request. I'm thus inclined to close it, especially if your ongoing Rock-on revamp work is to address the same thing, in part. And we will still have the work and observations @anatox and @MFlyer put into this.

[FroggyFlox]

@phillxnet,

No, it is not currently sorted by other means and the issue still stands. It is actually part of a "series" of re-work we need to do to make these kinds of settings work at the container level rather than the rock-on level (#2000, for instance).

It has unfortunately been a little while since I've looked seriously at this PR, but as far as I remember I thought @anatox made the right changes. The related code has not changed that much--mostly the surrounding code--but a rebase may not be too difficult (I honestly simply need to dedicate some time to it to properly test it).

Note that I do not currently have any ongoing work that would fix the related issue, and what @anatox has already written will most likely end-up being reused. I personally believe it is a "relatively" small fix that is rather expected by our users (myself included) based on the fact that this issue came up several times in the forum. As a result, it is rather high on my to-do list related to rock-ons.

I agree that the given the changes in the area, it may be difficult to make simple "edits" to this PR, however.

[phillxnet]

@FroggyFlox Thanks for straightening that one out.
@anatox I know this has been years (actually) but if you fancy stepping up once again, re @FroggyFlox comments, it would be good to have this re-based. Or just resubmitted on current master as that may end up being easier given all the changes that have happened in the mean time. No worries otherwise as we can, in time, get this sorted. But would be a help to have it freshly updated and would be easier to do accreditation then for what you've already sorted.

[anatox]

@FroggyFlox Thanks for straightening that one out.
@anatox I know this has been years (actually) but if you fancy stepping up once again, re @FroggyFlox comments, it would be good to have this re-based. Or just resubmitted on current master as that may end up being easier given all the changes that have happened in the mean time. No worries otherwise as we can, in time, get this sorted. But would be a help to have it freshly updated and would be easier to do accreditation then for what you've already sorted.

Thank you for interest to this issue again. I'm a bit busy right now but I would check it sometime next week.

[Hooverdan96]

It has been another 4 years now. @anatox would you still be interested in getting your PR straightened out (aka re-baseline, or a new PR? A brief look at the coding in that area, it seems nothing specifically has changed there, so it could be fairly straightforward.
If yes, I'd recommend to rebase against the testing branch at this time and not the master branch, since we're (meaning @phillxnet and @FroggyFlox primarily) in the midst of quite a few infrastructure/component upgrades, and this could then possibly make it into the next RC with those modernized other Rockstor components in place.

Or, if you don't have the time/mindshare, @FroggyFlox, should we consider creating a new PR with these changes in here (against the testing branch)?
To me, this seems to be a "low hanging" fruit that could be knocked out quickly and stabilize the multi-container Rockon options ...

[Hooverdan96]

In light of comments/interest on the forum about new Rockons that require multi-container setups, this might become a bit more important for installations like immich.app or photoprism or even ente for photo management as they have multi-container requirements.

[phillxnet]

Closing as:
Superseded by #2887