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.

Sign up for GitHub

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 "Invalid environment variabled" error for multi-container rockons with "environment" section #1688

Closed
wants to merge 2 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b36bb1e
Don't raise an exception for missing env.var.
anatox Apr 3, 2017
9643410
Fix handling of environment variables in multi-container rockons
anatox Apr 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions src/rockstor/storageadmin/views/rockon_id.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,13 +145,11 @@ def post(self, request, rid, command):
cco.val = cc_map[c]
cco.save()
for e in env_map.keys():
if (not DContainerEnv.objects.filter(
if (DContainerEnv.objects.filter(
Copy link
Member

@MFlyer MFlyer Apr 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Author

@anatox anatox Apr 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@MFlyer MFlyer Apr 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@MFlyer MFlyer Apr 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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 😃

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@anatox anatox Apr 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

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 😉

Copy link
Author

Choose a reason for hiding this comment

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

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.

container=co, key=e).exists()):
e_msg = ('Invalid environment variabled(%s)' % e)
handle_exception(Exception(e_msg), request)
ceo = DContainerEnv.objects.get(container=co, key=e)
ceo.val = env_map[e]
ceo.save()
ceo = DContainerEnv.objects.get(container=co, key=e)
ceo.val = env_map[e]
ceo.save()
install.async(rockon.id)
rockon.state = 'pending_install'
rockon.save()
Expand Down