Query networking actions

[naved001]

I think it's pretty much done, except migration scripts/tests. But I figured it might be a good idea to get some feedback while I work on that.

• Fix #486 and* #427 since they are duplicate issues.

[naved001]

one thing to note is that; if we were to allow queuing multiple actions on a nic later on, we would need to think about things like:

• for how long do we keep completed actions
• or how many completed actions do we keep
• do we keep them in the same NetworkAction table or a new one for completed actions.

I was looking at #510 and that reminded me that @apoorvemohan strongly believes that we should let users queue multiple ops on nics, and that users should deal with the consequences if any of their network actions fail.

[zenhack]

I think figuring out multiple queued network actions can safely be treated as a separate issue. I have a couple ideas about the design.

I'll do a review of this when I'm at my computer; on my phone right now.

[naved001]

@zenhack I have addressed some of the comments, I'll fix the the other issues too.

One thing that I need your help with is the migration tests. I am not exactly sure how to write the migration tests (and fix the old ones). There's already a test that runs for the networking action table. Like there's a method which says "it generated the sql dump" in the migrations folder? I thought that was generated with the pg_dump command. just little things like that, maybe I'll ping you on irc tomorrow if you are online.

[naved001]

@mosayyebzadeh @SahilTikale Can either of you be a second reviewer on this?

[zenhack]

re: dump generation, the wording is a bit sloppy; what is meant is that the function was used to create the objects that were in the database when pg_dump was run.

[zenhack]

Quoting Naved Ansari (2018-01-11 10:07:42)

But that is true here (revertport), isn't it? The other network calls node{connect, detach}_network also return 202.

This refers to the status of the request being made; in the case of node_{connect, detach}_network, the request is to do the attachment. Here, it is to report the status.

[naved001]

@SahilTikale @mosayyebzadeh ping

[naved001]

Hey @Izhmash, is there a reason why you made direct db calls in this method rather than using the API (like how the create_pending_aactions does) to make the objects?

[naved001]

@zenhack, it's passing the tests now with my most recent commit.

[ianballou]

@naved001 some notes about editing the migration scripts now that #931 has merged: To update your migration script, delete/move the old script, rebase, initialize the DB, and then generate the new migration script. Note that @zenhack had to set branch_labels = None in #931; you'll have to do the same in his migration script now that it is the latest one.

Also, double check that the revision numbers make sense to ensure that the script was generated properly.

[naved001]

@Izhmash thanks! just did that.

[naved001]

@zenhack @Izhmash @SahilTikale ping

[naved001]

@zenhack @Izhmash @xuhang57 I think addressed pretty much everything you guys pointed out.

One thing to note: since I am now doing logger.error in deferred.py, I had to remove the fail_on_log_warnings fixture from the corresponding unit test, because in that test I am deliberately raising a switch error ~and catching it.~ which is caught and handled by deferred.py

[naved001]

I wanted to append some more tests to tests/unit/deferred.py. Basically, I wanted to test adding a valid networking action on a nic with a failed networking action. In my test I use local_db.session.add(new_action), before closing and committing, to add a new action. But it fails with a DetachedInstanceError.

I read the docs for sqlachemy, but couldn't figure out what's wrong with my approach.

[naved001]

@zenhack I'll fix the stuff you pointed out. Do you happen to know about the DetachedInstanceError that I'm seeing? To put it simply, I am trying to use local_db in the main test to add a new action, but it gives me that error.

[zenhack]

@naved001, do you have the code you wrote somewhere for me to look at?

[naved001]

Sorry for the delay in response, I have the code somewhere on my work machine. I'll push it tomorrow so you can have a look.

[naved001]

@zenhack what I was trying to do earlier was flawed for other reasons.

I have updated tests/unit/deferred.py where I use the API to add a networking action on a nic with failed action. Running into some other problems, because I think there were some issues with the original logic of the test. Will push it soon.

[naved001]

@zenhack okay done. have a look at the latest commit and the message.

I don't know how 2 actions on the same nic got added (one with status = ERROR and one with status = PENDING); because we are enforcing 1 to 1 relation.

I think tests/unit/deferred.py could be refactored, but I'd prefer to not do it in this PR because it's pretty big as it is.

@xuhang57 @Izhmash i have added some more commits after your approval. Please have a look at it once.

[zenhack]

LGTM. If others are happy with the state of the new tests, we can merge.

[xuhang57]

lgtm

[naved001]

alright, merging.

[naved001]

yay! finally no. 2 on the hil leaderboards.