Opened 7 years ago

Closed 7 years ago

#2710 closed defect (fixed)

b10-cmdctl needs restart when adding users

Reported by: jelte Owned by: jelte
Priority: medium Milestone: Sprint-20130319
Component: ~bind-ctl (obsolete) Version:
Keywords: Cc: cas
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 1.05 Internal?: no

Description

When adding a user with b10-cmdctl-usermgr, b10-cmdctl needs to be restarted.

It calls _create_user_info(), but that code then skips the reading if it has already read it:

        if (self._accounts_file == accounts_file) and (len(self._user_infos) > 0):
            return

While I can appreciate the attempt to skip unnecessary re-reads, if we do so we should at the very least check whether the file has changed.

Subtickets

Change History (12)

comment:1 follow-up: Changed 7 years ago by shane

So, is the proposal to have something like:

    if (filecmp.cmp(self._accounts_file, accounts_file) and ...):
        return

comment:2 Changed 7 years ago by shane

  • Cc cas added

comment:3 in reply to: ↑ 1 Changed 7 years ago by muks

We'd have to keep around an mtime of self._accounts_file:

        new_mtime = os.stat(accounts_file).st_mtime
	if (self._accounts_file == accounts_file) and (self._accounts_file_mtime >= new_mtime):
	    return
        self._accounts_file_mtime = new_mtime

The ticket may need more work inside cmdctl.py.in.

comment:4 Changed 7 years ago by jwright

  • Component changed from Unclassified to bind-ctl
  • Milestone changed from New Tasks to Next-Sprint-Proposed

comment:5 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130319

comment:6 Changed 7 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:7 Changed 7 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

ready for review.

i made the original _accounts_file (which stores the file name) 'private', but left the other members alone (some of those could probably be private as well, though a number of them are directly used in tests).

Also, I made it behave slightly better if the file doesn't exist in the first place. In theory this shouldn't happen from the 'config' point of view (as this is checked), but the file could for instance be removed, etc. In that case all stored user data is removed.

comment:8 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:9 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jelte

Hello

The method looks good enough, but there are few minor nits:

Why is the file initialized for None, but then, if the file does not exist, it is still set to the path of the file that does not exist?

Why not output all the lines at once here? Something like joining them with "\n" and writing?

    def __enter__(self):
        with open(self.__path, 'w') as f:
            for line in self.__contents:
                f.write(line)

Is the second assert here of any use? If the dict is empty, then 'root' definitely is not in it.

        self.assertEqual(0, len(self.server._user_infos))
        self.assertFalse('root' in self.server._user_infos)

comment:10 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to vorner

Replying to vorner:

Hello

The method looks good enough, but there are few minor nits:

Why is the file initialized for None, but then, if the file does not exist, it is still set to the path of the file that does not exist?

in the (probably nonexistent, due to config checks) case that it is changed to something that does not exist; however it should indeed just be reset to None in that case. Changed.

Why not output all the lines at once here? Something like joining them with "\n" and writing?

    def __enter__(self):
        with open(self.__path, 'w') as f:
            for line in self.__contents:
                f.write(line)

OK

Is the second assert here of any use? If the dict is empty, then 'root' definitely is not in it.

        self.assertEqual(0, len(self.server._user_infos))
        self.assertFalse('root' in self.server._user_infos)

heh :) no, indeed. Removed :)

comment:11 in reply to: ↑ 10 Changed 7 years ago by vorner

  • Owner changed from vorner to jelte
  • Total Hours changed from 0 to 1.05

Hello

Replying to jelte:

Why not output all the lines at once here? Something like joining them with "\n" and writing?

    def __enter__(self):
        with open(self.__path, 'w') as f:
            for line in self.__contents:
                f.write(line)

OK

I looked at this part again and discovered it is missing the end of the last line. Actually, the original code was missing all the end of lines, I guess. I fixed and pushed to the branch, I hope you don't mind.

It looks OK to merge with this.

comment:12 Changed 7 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed

oh I merged this yesterday and forgot to close it

thanks for the review!

Note: See TracTickets for help on using tickets.